guiu.rocafort.ferrer → created an issue.
I have migrated the following permissions and updated the code accordingly:
- browse translations
- export gettext templates and translations
- decline own suggestions
There is a permission ( start over packages ) which is not used anywhere, so just removed it for now.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
There pipeline failures are already present in the 3.0.x branch of the module.
guiu.rocafort.ferrer → created an issue.
I think the main problem in this issue was the unhandled errors that would cause a WSOD installing a module ( whether the translations folder is not correctly set or the download failed, or other reason ) and this merge request fixes this.
I agree with @drumm that we should keep it separate, and use the other issue he created to provide more accurate message errors for the different scenarios, so we can move on and fix this issue.
Hi @das-peter ! Thank you for pointing out that the merge request was not up to date. I merged the development branch into the MR one, so now it should be good to go. Could you test the MR branch and mark the issue as RTBC if everything is ok so i can merge it and fix the issue ?
Thanks for the review d.fisher !
Merging and marking as fixed.
Doing a few tests around i realised that setting the country path in the hostname does not make a lot of sense conceptually, so i went back to the first branch that used that approach.
I also realised that a method to get the request path without the active domain path was needed, because if not, the links for the other domains would also contain the active domain country path, so i created a new method to get the active domain country path.
I also added a functional test that checks the navigation links. Setting as needs review.
guiu.rocafort.ferrer → changed the visibility of the branch 2961459-country-path-not to active.
Setting priority to critical as it breaks completely the functionality of the module.
The call to domain createInstance parent method was causing the CountryPathDomainListBuilder being called with 7 parameters instead of 8 because of the "new static" call in it.
guiu.rocafort.ferrer → changed the visibility of the branch 3.x to hidden.
Targeting now version 3.x-dev using config collections, setting as needs work.
guiu.rocafort.ferrer → created an issue.
When the value is bigger or smaller than the PHP_INT_MAX/MIN values, it was getting modified in the Drupal\Core\TypedData\Plugin\DataType\IntegerData::getCastedValue to the max/min value of php in that architecture, so i tried a different approach and added a check that throws an exception when the value is not within the valid value range for the system.
I've done it to the integer and also to the float values, and wrote tests for it.
The pipeline is failing right now, but the tests that are failing seem to be unrelated to the changes made in the branch.
So i went back to the 11.x development branch and did a site install with standard profile. Then i exported the configuration and changed the automated_cron.settings interval attribute ( which has a zero or positive constraint ) to a negative value. When i import the configuration it does not throw any error, but when checking the object using the config_inspector module it does show a validation error. Not sure if this is te expected behaviour or something else is failing.
Hi @bbrala. Thanks for the test code and the suggestions !
The main issue is that i have not found a way to validate this manually, apparently the constraint is applied to the configuration ( at least according to config_inspector ), but when i modify the configuration yml to an invalid value and try to import it, it does not throw any validation error, so i guess there is something i am still missing to make it work.
Later i plan on fixing the code style issues and writting tests.
Hidden one of the branches since the country-path-not-setHostname-handling is more up to date with the latest changes.
I think we should leave the workaround for the domain module infinite loop at least until there is a release including that fix, then we can change the minimum version requirements for the domain module to make sure it has at least the released version with the fix.
I have been testing the branch and seems to work ok, but i think we should write some tests for this.
guiu.rocafort.ferrer → changed the visibility of the branch 2961459-country-path-not to hidden.
I have managed to create a constraint, that is based on the Range one, and defines the min and max values using the php constants PHP_INT_MIN and PHP_INT_MIN. I've validated that the constraint is applied to the configuration value ( using automated_cron.settings interval attribute ), but i don't seem to manage to trigger the validate method for the constraint on configuration import.
I was able to reproduce the issue and tested that the patch solves it.
I corrected the OpeningFileError because it was meant to test that calling readItem without opening the file would return NULL.
Then i realised that the fgets returns a TypeError exception, even though it is not documented in the php function page ( https://www.php.net/manual/en/function.fgets.php ). Also, this exception was not being thrown one month ago ( https://git.drupalcode.org/issue/drupal-3301239/-/pipelines/552305 ) so i asume that something was changed in the way the tests are executed in the pipeline.
I modified the test to expect this exception, and now the pipeline is passing. Setting as needs review.
Looks like one of the tests is failing again:
✘ Opening file error
┐
├ Exception: Cannot open stream for uri fake
│
│ /builds/issue/drupal-3301239/core/lib/Drupal/Component/Gettext/PoStreamReader.php:158
│ /builds/issue/drupal-3301239/core/tests/Drupal/Tests/Component/Gettext/PoStreamReaderTest.php:49
┴
I will give it a look.
Should be ready for review now.
Changed the class name to DomainConfigUtilities to avoid future conflicts when using config collections.
Added back the method getConfigNamesByDomainAndLanguage as a static method in the class.
Hello, i made a new branch because i kind of messed up doing the merge in the original one... sorry about that.
I tried to take a new approach, where i created an auxiliary method in the module file and then it can be called from the new service and the existing overriding one. Since the new method getConfigNamesByDomainAndLanguage only really calling the new method, i thought it would be ok to remove it, since i think it has not been included in any release yet so we are not really doing any API changes from the existing ones.
We could also add it back, but i don't really see the point of it.
Hi mably, i checked your change and looks ok to me. I actually think that It is better to do It this way. I think you can go ahead and merge the branch from my part.
Thank you !
guiu.rocafort.ferrer → created an issue.
You are right @mably, that check did not make much sense. I changed the test to check if the "Enable domain configuration" link is not present instead.
I missed that part, setting to needs work to remind me to fix it later.
The phpcs validation is passing now. For some reason the phpunit starting failing because it is trying to test it against Drupal 11, and the module does not support 11.
Please review
I have changed the hook to be _info, so modules can return their forbidden paths, without interacting with the paths returned by other modules.
Regarding the way of checking if the "Enable domain configuration" is present on the page, i have reused the same strategy from other tests in the module, which do it that way already.
I have implemented a hook that allows other modules to add forbidden paths for config_ui.
I also implemented a test for the new functionality.
Please review.
I assigned some credit still for the effort of writing and reviewing the patches. Thanks everyone !
I am marking this as duplicated of #2961459, the issue seems to be that the url for the domain is not correctly generated, so the path for the domain is not added to the url. This is why it feels like it is using the default domain, when in fact it is not generating the domain url properly.
I tested the patch in #2961459 and the issue is fixed.
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.