Hi @idebr, true, i missed that, thanks for the comment, i made the change to string.
This fix is included in the latest 8.x-1.0-rc5 version.
Yes, exactly.
Tested it and works well
guiu.rocafort.ferrer → created an issue.
I've added the global constraint validation errors on the top of the List and Tree pages, but it stills needs to be displayed in a better way.
guiu.rocafort.ferrer → created an issue.
I fixed some obvious and easy phpstan and cspell errors.
I did not manage to understand why cspell is throwing this error:
src/Form/BundleSettingsForm.php:51:48 - Unknown word (Inferface) -- Entity\ContentEntityTypeInferface
Suggestions: [interface, Interface, Inference, Interlace, Interrace]
There are some phpstan errors in CopyLayout.php which did not seem so trivial, so i left them out. They can be fixed in a separate issue
166 Parameter $original of method
Drupal\content_entity_clone\Plugin\content_entity_clone\FieldProcessor\CopyLayout::cloneBlockContentEntity()
has invalid type
Drupal\content_entity_clone\Plugin\content_entity_clone\FieldProcessor\BlockContentInterface.
174 Access to an undefined property
Drupal\content_entity_clone\Plugin\content_entity_clone\FieldProcessor\CopyLayout::$blockContentStorage.
💡 Learn more:
https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
This issue seems to be outdated, i was not able to reproduce it in dev branch.
Works good for me.
We might want to add additional validation for the ContentEntityCloneFieldProcessor plugins ( see https://www.drupal.org/node/3330852 → ), but this would require to bump up the minimal Drupal version to 10.1 in the next release.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
More descriptive title and menu description.
Steps to reproduce:
- Install Country Path 8.x-1.x ( dev branch ).
- Enable Domain and Country Path.
- Go to "/admin/config/domain" and create multiple domains.
- Navigate to the different domains, validate they load properly.
- Apply this patch to the Country path module (
https://www.drupal.org/project/country_path/issues/2961459
🐛
Country path not managed in domain navigation block
Needs work
).
- Navigate to any page in any domain. There is an infinite loop and the pages fail to load.
Then, apply the merge request diff ( https://git.drupalcode.org/project/domain/-/merge_requests/135.diff ) to domain module and clear caches. The domains should load properly now.
Updated patch against lates 8.x-1.x branch.
This has been fixed in this issue: https://www.drupal.org/project/country_path/issues/3429638 📌 Automated Drupal 11 compatibility fixes for country_path Needs review
It will be available on the next stable release, which i am planning to release by the end of march.
guiu.rocafort.ferrer → created an issue.
Removing the needs test tag, think @roderik is right.
Hi @roderik! Thanks for your comments.
I believe there are ( at least ) three methods that throw exceptions that are not handled in the validateValues method.
1. $this->checkValues($config_name) can throw the following exceptions:
InvalidArgumentException ( it does that in several classes extending TypedData class ).
MissingDataException ( some classes extending TypedData class ).
2. typedConfigManager->getDefinition can throw the following exceptions:
PluginNotFoundException
3. TypedData->validate can throw the following exceptions:
LogicException
UnexpectedTypeException
InvalidArgumentException ( not sure if it is really thrown, since it might not execute because the checkValues would fail ).
MissingDataException ( not sure if it is really thrown, since it might not execute because the checkValues would fail ).
The phpdoc for the validateValues method claims it can throw a SchemaIncompleteException, although i am not sure where that would come from.
In summary: The exceptions in those three methods should be handled in the validateValues, and appropiate errors returned. Because there are so many types of exceptions, and there are not really well documented in the phpdoc comments for the methods, i think we might just catch all the Exceptions for now to be sure we don't miss anything, although i am aware this is not really a good practice...
I think this should allow to close a few similar issues already existing:
https://www.drupal.org/project/config_inspector/issues/3444730
🐛
Expected argument of type "array", "string" given
Active
https://www.drupal.org/project/config_inspector/issues/3456858
🐛
The LangcodeRequiredIfTranslatableValues constraint can only operate on the root object being validated.
Needs work
guiu.rocafort.ferrer → created an issue.
I have added a try / catch so the exceptions thrown in the validation does not cause a fatal error which interrupts the rest of the configuration validation. Setting the issue as needs review.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
I've approached the issue catching the Exception from SchemaCheckTrait::checkConfigSchema and returning an array with the error message from that exception in checkValues.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
The code now checks if the widget is of type checkbox/select, or if the widget is autocomplete.
If the code is checkbox/select, it does the same as before, but with an autocomplete field, it initializes the selection plugin and calls countReferenceableEntities() instead.
Regarding the 'Chosen' module, i have not tried it, but it should not throw an error and just not hide the field if there is no domain available.
@vladimiraus, i appreciate the refactor and improving the code, it looks way better this way, but please, next time try not to introduce validation errors, it was passing all the tests before the change. I fixed it already.
@pfrenssen I guess that now that the open method returns an exception when it cannot open the file, it is way less likely that readItem is called, but it is still not impossible. I think it would be a good idea to allow readItem to throw an exception when the fd is not valid, but i am worried that this might break existing code which is not expecting the method to be thrown.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Thanks @pfrenssen for fixing the failing test.
I've tried again enabling a module with the translations folder having the wrong permissions and it outputs the following messages:
> [notice] Checked de translation for webform.
> [notice] Checked es translation for webform.
> [notice] Checked ca translation for webform.
> [error] Unable to download translation file https://ftp.drupal.org/files/translations/all/webform/webform-6.3.0-beta1.de.po.
> [warning] filesize(): stat failed for /var/www/html/web/sites/default/files/translations/webform-6.3.0-beta1.de.po locale.bulk.inc:204
> [notice] Unable to import translations file: translations://webform-6.3.0-beta1.de.po
> [error] Unable to download translation file https://ftp.drupal.org/files/translations/all/webform/webform-6.3.0-beta1.es.po.
> [warning] filesize(): stat failed for /var/www/html/web/sites/default/files/translations/webform-6.3.0-beta1.es.po locale.bulk.inc:204
> [notice] Unable to import translations file: translations://webform-6.3.0-beta1.es.po
> [error] Unable to download translation file https://ftp.drupal.org/files/translations/all/webform/webform-6.3.0-beta1.ca.po.
> [warning] filesize(): stat failed for /var/www/html/web/sites/default/files/translations/webform-6.3.0-beta1.ca.po locale.bulk.inc:204
> [notice] Unable to import translations file: translations://webform-6.3.0-beta1.ca.po
> [notice] The configuration was successfully updated. 398 configuration objects updated.
> [error] Message: Failed to save file due to error "/The specified file
> 'temporary://fileB2wo3R' could not be copied because the destination
> directory 'translations://' is not properly configured. This may be caused by
> a problem with file or directory permissions./"
>
> [error] Message: Failed to save file due to error "/The specified file
> 'temporary://fileSQbYRB' could not be copied because the destination
> directory 'translations://' is not properly configured. This may be caused by
> a problem with file or directory permissions./"
>
> [error] Message: Failed to save file due to error "/The specified file
> 'temporary://fileUBLW8Q' could not be copied because the destination
> directory 'translations://' is not properly configured. This may be caused by
> a problem with file or directory permissions./"
>
> [error] Message: 6 translation files could not be imported. See the log for details.
>
> [notice] Message: The configuration was successfully updated. There are /398/ configuration
> objects updated.
>
The command does not fail, but it also displays enough error messages to point up to the user there is a problem with the translations folder permissions, there are also test for validating the changes made, so i am marking it as needs review.
One of the tests is failing because php is throwing a warning message, and it makes phpunit treat fail the test...
Hi @pfrenssen, thanks for your comments.
I think a good approach would be to throw an exception in open when fopen fails to open the stream.
Regarding readItem, i would leave the readLine fix still there, in case someone calls the method accidentally. The method might already return NULL if the file has some errors in it or if it is empty, so i think there is no warm in leaving it for now.
I will add the exception to the open method, and also leave the already implemented fix in readLine, and add a new test to validate the open exception.
When this issue is closed, we can create a separate issue to review the whole PoStreamReader class and add more exhaustive unit testing to all the methods.
I created a new branch from the targeted Drupal version 11.x in the issue and applied the previously commited change in the old branch by @immaculate.x. I reproduced the issue with the change, and now the command "drush pm:enable paragraphs" succeeds, but it gives the following errors:
> [notice] Checked es translation for paragraphs.
> [notice] Checked it translation for paragraphs.
> [notice] Checked de translation for paragraphs.
> [error] Unable to download translation file https://ftp.drupal.org/files/translations/all/paragraphs/paragraphs-8.x-....
> [warning] filesize(): stat failed for /var/www/html/web/sites/default/files/translations/paragraphs-8.x-1.18.es.po locale.bulk.inc:204
> [warning] fopen(translations://paragraphs-8.x-1.18.es.po): Failed to open stream: "Drupal\locale\StreamWrapper\TranslationsStream::stream_open" call failed PoStreamReader.php:154
> [notice] Unable to import translations file: translations://paragraphs-8.x-1.18.es.po
> [error] Unable to download translation file https://ftp.drupal.org/files/translations/all/paragraphs/paragraphs-8.x-....
> [warning] filesize(): stat failed for /var/www/html/web/sites/default/files/translations/paragraphs-8.x-1.18.it.po locale.bulk.inc:204
> [warning] fopen(translations://paragraphs-8.x-1.18.it.po): Failed to open stream: "Drupal\locale\StreamWrapper\TranslationsStream::stream_open" call failed PoStreamReader.php:154
> [notice] Unable to import translations file: translations://paragraphs-8.x-1.18.it.po
> [error] Unable to download translation file https://ftp.drupal.org/files/translations/all/paragraphs/paragraphs-8.x-....
> [warning] filesize(): stat failed for /var/www/html/web/sites/default/files/translations/paragraphs-8.x-1.18.de.po locale.bulk.inc:204
> [warning] fopen(translations://paragraphs-8.x-1.18.de.po): Failed to open stream: "Drupal\locale\StreamWrapper\TranslationsStream::stream_open" call failed PoStreamReader.php:154
> [notice] Unable to import translations file: translations://paragraphs-8.x-1.18.de.po
> [notice] The configuration was successfully updated. 370 configuration objects updated.
> [error] Message: Failed to save file due to error "/The specified file
> 'temporary://fileSRCiNi' could not be copied because the destination
> directory 'translations://' is not properly configured. This may be caused by
> a problem with file or directory permissions./"
>
> [error] Message: Failed to save file due to error "/The specified file
> 'temporary://filePFV9Fz' could not be copied because the destination
> directory 'translations://' is not properly configured. This may be caused by
> a problem with file or directory permissions./"
>
> [error] Message: Failed to save file due to error "/The specified file
> 'temporary://fileWiy0wK' could not be copied because the destination
> directory 'translations://' is not properly configured. This may be caused by
> a problem with file or directory permissions./"
>
> [error] Message: 6 translation files could not be imported. See the log for details.
>
> [notice] Message: The configuration was successfully updated. There are /370/ configuration
> objects updated.
Notice that the errors already says that the directory translations might be not properly configured, so i think this should be good enough to not generate a WSOD, and also display the appropiate error messages to the user about the translation folder not being correctly configured.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
I made a few adjustments to the composer.json file to fix the pipeline execution.
I also confirmed the original issue with the composer require is fixed.
Merging the issue, thanks everyone !
I managed to reproduce this error when using a "Autocomplete" or "Autocomplete (Tags style)" for the field "Domain Access" in the manage form display configuration for the content type.
Clearly the code does not contemplate the possibility of using those field widgets for this field, so the patches do not fix the root cause of the issue.
@dastan56 Can you specify the exact composer command you tried to execute ? I cannot reproduce this myself.
Discussing the issue in Slack #drupalpod channel, @rfay has suggested that we might try to mount the necessary files inside the web container.
Also @shaal has confirmed he can replicate this issue.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
After updating the branch to be able to use the CI pipelines, it turns out the test is not quite right, and needs to be fixed.
guiu.rocafort.ferrer → changed the visibility of the branch 3429638-automated-drupal-11 to hidden.
Thank you @idebr !
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
I found something interesting experimenting around:
I added a value to a configuration value of type weight ( in this example user.role.authenticated.weight ), bigger thant the PHP_INT_MAX value for 64 bit ( 922337203685477580722 ). The PHP_INT_MAX for 64 bit is 9223372036854775807.
Imported the configuration via drush cim ( it did not show any error ).
Export the configuration via drush cex.
Then i saw that the integer value that was exported, was set to the PHP_INT_MAX for 64 bits ( 9223372036854775807 ).
So, isn't the configuration validation suposed to throw an error when importing invalid configuration ?
Also, if this is confirmed, the imported value should not be modified after the import, so if the value is bigger that the PHP_INT_MAX value for that machine, the configuration should not be valid. In that case, i guess the validation should be implemented in a callback so the specific PHP_INT_MAX/MIN values for that php can be validated.
PD: I tried this in Drupal 11.0.5, i will try to replicate this using the drupal dev branch.
I have applied the changes in a new merge request to 7.x-1.x (3472254-wrongly-parsed-sentence-7.x).
@fmb I have checked that in the frontend the variables are not incorrectly hightlighted by the javascript code.
The issue fork have been merged in development. Marking as fixed.
I tried again in my local machine today ( yesterday i tried with a drupalpod instance ), and the redirecting issue seems to be a thing with the DrupalPod integrated browser, sorry about that.
The issue needs tests before it can be merged.
I have checked that the issue is still present in the current 8.x-1.x-dev branch, and that the patch fixes the redirection issue.
The problem i see is that, even though the loaded domain is the default one, the url is the same, i think that at least a redirect should be done to the domain that is actually being loaded, so i am setting this as needs work.
Tasks pending:
- Make an actual redirection so the url in the browser matches the displayed domain.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
@_tarik_ i don't think that storing empty third_party_settings around is a good idea. I am not 100% sure, but it might also introduce unnecesary dependencies in the domain not using the country_path functionality to the module. I think a better approach might be to alter de JSON:API response to include the empty values in the response, for example.
I am setting this issue as won't fix.
I am interested in co-mantaining the project, so i am changing the status to active again.
The local tasks menu now displays a "Languages" link in it. Setting it to needs review.
Hello @kartagis, thank you for the video, it was very helpful.
When you are adding the translation, now there are two different text areas. You are suposed to add the individual string translation in one, and the plural string translation in the second one.
I am attaching a screenshot to try clarify it. of couse i don't know turkish so i just copied the text in english to ilustrate the point.
Shouldn't the range constraints of PHP_MIN_INT and PHP_MAX_INT be defined in the integer type ?
We might then try to validate the configuration using the configuration value "system.site.weight_select_max", but i am not sure if this is possible... aside from having an adequate constraint plugin, it might require to add system.site to as a dependency to all of the configuration objects that uses type: weight
Thanks for looking at this.
This was fixed and available on localize.drupal.org for a while.
I tested it and added a translation for the string mentioned, and it works properly. I am setting the issue as fixed because of that.
We found out that the code was treating JSON:XXXXXX as a variable, this is a general problem where it is interpreting JSON:API as a variable name, when it is actually the drupal core submodule name.
I added an exception to the regular expression checking the variables, that would ignore any value starting with "JSON:" from being interpreted as a variable.
guiu.rocafort.ferrer → changed the visibility of the branch 3472254-wrongly-parsed-sentence to hidden.
The string id i wrote in the comment #7 was wrong, i did not realise the id in my local setup is different than the one in localize.drupal.org, so İ updated the comment accordingly.
Sorry about that.
I was not able to replicate this issue in a local environment:
I used main branch from drupal-infrastructure gitlab repository,
I used 3.0.x-dev branch for l10n_server module.
The string id i tried to translate, and had no problem is the #5052.
Since issue #3423189 have been fixed, granting my account the possibility of opting projects into security advisore coverage, i am moving the issue back to Drupal.org project ownership so it can be resolved.
Hi @avpaderno,
I removed the hook_form_alter. Please review.
Thank you for your patience.
guiu.rocafort.ferrer → created an issue.
Hi @avpaderno
I have removed the buildConfigurationForm and implemented a form_alter hook instead.
Marking as needs review.
I believe that the fix should be merged, so the breakString does not return a NULL operator, which causes a fatal php error, and then we can create a follow-up issue to discuss the acceptable characters in the provided values, as #59 suggests.
The merge request solves the issue implementing hook_module_implements_alter, and making sure the migration_plugins_alter hook from migration_plus executes before anything else. This way, when the core action module executes its migration_plugins_alter hook, the migration configuration will already include the merged shared_confguration from the group.
Setting the issue as needs review.
I have updated the test files since there were not the correct ones.
Hi @avpaderno
The thing is that this is made on purpose, and it is part of how this module work and makes sense. The fact that the configuration is in a separate settings form, allows the submodule from domain (domain_config) to override the settings for different domains. If i would add the configuration form at the block itself, then this module would not have any reason to exist, because the "Social Media Links Block and Field" module ( https://www.drupal.org/project/social_media_links → ) would have the same functionality.
If this is not acceptable to grant the security advisory coverage, let me know and we can close the issue, so we don't waste each other's time. I will try to qualify for the security advisory coverage when i have a chance to contribute another Drupal module and open a new issue.
So if i remove the buildConfigurationForm method for the class and implement a form_alter hook, would this be considered a better approach for this use case ? @avpaderno
The patch worked for me in Drupal 10.3.1
Hi @avpaderno,
I am a bit confused about what you are saying. SocialMediaBlock::buildConfigurationForm() is getting the form from the parent, and then adding a link element before returning it. The only reason i could think of this not being a good practice, might be the fact that the function is not adding any form element, it is just adding a link. I could remove the buildConfigurationForm method and add a form_alter in the .module file to add the link instead, would that be a more acceptable practice ?
In my case, i had a string value with the character "/" on it, so the regular expression was not picking it up and causing the operator to be NULL.
I think the main issue comes from HandlerBase::breakString, which should allways return an operator, even if none of the regular expressions apply. In that case, the value returns as an empty array, so i believe it should be safe to return any of the operators for that matter.
This issue does not only happen with numeric arguments, but also with string arguments that contain characters not present in the regular expression, so the fix should not focus on the numericArgument plugin.
Please review the new branch '3353786-breakstring-add-default-operator'. Setting the issue as needs review.
PD: Another different issue, which will require a different issue to address, is broadening the regular expression to allow for more special characters to be present, like the one that was causing the issue for me "/".
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Changing issue status to needs review.
Hi @avpaderno.
Thank you again for your feedback.
I have addressed the translatable elements issue as requested.
Please notice that i added a @codingStandardsIgnoreLine because phpcs did not seem to understand that the value of the variable is comming from a literal ( defined as a CONST in the class itself ), and complained about "Only string literals should be passed to t() where possible".
Regarding the block configuration, i am aware that this might not be the habitual way of handling it, but it is completely intentional, and part of how the module works. This module defines a block, that uses some settings defined outside the block itself. This is made on purpose to allow the domain_config subdomain to be able to define different variations of it for different domains. The link in the block settings form is merely a reminder for people that might be confused by how the module works, since there is another similar module "Social Media Block and Field" that works placing the configuration in the block itself. Please read the module description and let me know if you have any further questions about this issue.