I'm fine because we made now everything faster in 📌 Improve ::getStrings() performance Needs review
claudiu.cristea → made their first commit to this issue’s fork.
What is covered:
The performance penalty is caused by the module's architecture, which compiles a long list of string translations by concatenating strings extracted from plugins. This is fragmenting the strings fetching from database.
A big problem is the config plugin which has a bottleneck when parsing the config translatable properties. Even the config translatables are cached, the method is called for each config adding a lot of time and overhead. This needed a special attention
To address the severe performance penalty, I've implemented caching of strings per-language. Each language creates its own cache entry.
The best architecture for this it turned to be a "strings cache collector", which allows to resolve cache misses on the fly.
Technical:
claudiu.cristea → made their first commit to this issue’s fork.
I'm coming from 1.1 and this fixes the issue. Without this, there is an "User" text after the user's message and the assistant name before the welcome text. This makes it comply with 1.1, which is what I've expected
claudiu.cristea → created an issue. See original summary → .
It the property name, placeholder_url, might be confusing because it makes you think that the value should be a URL. But it's the placeholder text for a URL field, for instance "Fill in the target URL (e.g., http://example.com". And that for sure should be translatable.
The fix is obvious.
Regarding failing the authorization request when at least one scope is not provided, either by the request or by the default scope set on the consumer, this is a valid approach for the authorize controller.
This means default scopes will be merged with scopes passed to request in order to get the final request scopes?
claudiu.cristea → made their first commit to this issue’s fork.
claudiu.cristea → changed the visibility of the branch 11.x to hidden.
claudiu.cristea → changed the visibility of the branch 2605218-views-block-display to hidden.
claudiu.cristea → changed the visibility of the branch 2605218-views-block-preblockbuild-ajax to active.
claudiu.cristea → changed the visibility of the branch 2605218-views-block-preblockbuild-ajax to hidden.
claudiu.cristea → changed the visibility of the branch 2786587-thread-depth-limit-11.x to hidden.
claudiu.cristea → changed the visibility of the branch 2786587-thread-depth-limit-9.5 to hidden.
claudiu.cristea → changed the visibility of the branch 2786587-thread-depth-limit-10.1 to hidden.
claudiu.cristea → changed the visibility of the branch 2786587-thread-depth-limit to hidden.
claudiu.cristea → changed the visibility of the branch 9.2.x to hidden.
claudiu.cristea → changed the visibility of the branch 11.x to hidden.
claudiu.cristea → changed the visibility of the branch 10.3.x to hidden.
claudiu.cristea → changed the visibility of the branch 9.4.x to hidden.
claudiu.cristea → changed the visibility of the branch 2786587-thread-depth-limit-9.2.x to hidden.
claudiu.cristea → changed the visibility of the branch 2786587-thread-depth-limit-10.3 to hidden.
Re #161:
1. Updated the IS. Mentioned the trait.
2. There are only 2 changes outside the comment module. Both are test expectations changes because of the comment module changes.
3. Removed EntityCreateAccessCustomStaticCidTest because in the meantime, core added a similar test.
Ready for review
claudiu.cristea → changed the visibility of the branch 2879087-comment-access-handler-10.4.x to hidden.
Of course, some tests are failing now. With client_credentials grant type, if the scopes were already configured in the consumer, it was not possible to pass an additional one in request. But authorization_code didn't had this constraint. But if we apply the same policy to authorization_code I don't think it is a BC break, as, until now, it wasn't possible to store scopes inside consumer for this grant type.
This needs to get some feedback from the maintainers.
In one of our projects, we have a consumer factory and we need to assign scopes to consumers that have also the Authorization Code grant type. Client should not be aware of scopes when performing the request because scopes should be already contained in the consumer. This issue is allowing that (I think in earlier versions of the module this war possible as I saw in one of the YouTube tutorials).
Regarding the failing test, I'm not sure what is the expectancy.
Given this requirement...
If the client omits the scope parameter when requesting
authorization, the authorization server MUST either process the
request using a pre-defined default value or fail the request
indicating an invalid scope. The authorization server SHOULD
document its scope requirements and default value (if defined).
I think we should fail when at least one scope is not provided either by request, or as default scope set in the consumer. Or I'm missing something?
claudiu.cristea → made their first commit to this issue’s fork.
claudiu.cristea → changed the visibility of the branch 8.9.x to hidden.
claudiu.cristea → changed the visibility of the branch 2765065-bundle-plurals to hidden.
claudiu.cristea → changed the visibility of the branch 2765065-bundle-plurals-9.4.x to hidden.
claudiu.cristea → changed the visibility of the branch 2765065-bundle-plurals-d10 to hidden.
This is tricky as phpoffice/phpspreadsheet is performing some changes to strings when they are inserted in the spreadsheet. One example is \r\n -> \n but it seems there are others. It could become very risky in the sense that you might not be able to determine the Babel source by rehashing the strings from spreadsheet. I will keep this open for future evaluation.
claudiu.cristea → made their first commit to this issue’s fork.
The whole logic is here https://git.drupalcode.org/project/plural_serialization/-/blob/1.x/src/E...
Here's a module that might save anyone who want clean plural variants in the exported YAML: https://www.drupal.org/project/plural_serialization →
(maybe the idea could be moved in core?)
Thank you all. Merged
The PHP 8.4 test is failing
@bojan_dev, as this is critical, could we get a release including this?
Thank you!
Good!
I've added some work done previously just to not lose it. It's on top of 📌 Better spreadsheet protection Active .
Bonus: enabled tests
claudiu.cristea → created an issue.
claudiu.cristea → created an issue.
I don't know whether is related to this. I set the default language to other than English. Then I've exported the config (drush cex. Reinstalled the site from config but the English language is gone. Checked the config sync directory, I can see the language.entity.en.yml file. Checked the {config} table, no language.entity.en at all. It just vanished.
Covered in MR:
- Added the fix
- Update path
- Regression test
Ready for review
claudiu.cristea → created an issue. See original summary → .
claudiu.cristea → created an issue.
Since last review:
- Removed the protection of column A to allow copy/paste of the string.
- Changed the table header to make clear which headers should not be edited
- Refactored
Spreadsheet::createExportedFileContent()to improve code readability - Adapted the tests.
Note that has validation has been previously fixed in 🐛 Import doesn't show collected warnings Active but violations are rendered as warnings.
claudiu.cristea → created an issue.