Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Its not possible to compare to TranslationWrapper objects, due the circular dependency between LanguageManager and TranslationManager. This patch removes circular dependency.
Proposed resolution
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Draft a change record for the API changes | Instructions |
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#40 | 2571375-40.patch | 28.39 KB | stefan.r |
|
Comments
Comment #2
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #3
alexpottOh wow... I like this.
Comment #4
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedRemoved ::t() altogether.
Comment #5
plach"Oh wow... I like this." (cit)
Comment #7
stefan.r CreditAttribution: stefan.r commentedThis is great, it removes the recursion that was bothering us in the var_export() and equality checks :)
Comment #8
XanoComment #9
chx CreditAttribution: chx commentedyay for less circular dependencies circular less for yay
Comment #10
alexpottThis would be good to test.
Comment #12
Gábor HojtsyComment #13
mr.baileysWorking on tests.
Comment #14
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #15
mr.baileysLet's see if this fails.
Comment #16
alexpottSo this is a bit more complex - I think we need to decouple the LanguageManager and TranslationManager completely. Discussed with @mr.baileys - I'm going to give this a shot.
Comment #19
alexpottThe patch attached completely decouples TranslationManager from LanguageManager as well. This makes comparing TranslationWrappers much lighter. As the only injected objects it has are the
Translators
.In order to do this we have to move TranslationManager::getNumberOfPlurals() to a service provided by locale - which makes perfect sense since it has little meaning of locale is not installed.
No interdiff provided because this is a major rework of the existing patch in #8 and it includes the tests (though converted to a KernelTestBase) from #15.
Comment #20
alexpottThis makes me happy. Less services required during very very early installer.
Comment #21
Gábor HojtsyI discussed this new approach with @alexpott, and it makes total sense architecturally AFAIS.
Comment #24
alexpottRerolled patch included an incorrect change in AccountForm that has since been changed in HEAD.
Comment #26
alexpottUpdated to use TranslatableString and fixed up installer tests.
Comment #29
alexpottFixing more tests
Comment #31
alexpottFixing tests.
Comment #32
dawehnerI really like the idea of decoupling!
I'm pretty convinced that forumla is the much nicer name. Did you solved the forumla? ... What are the forumlas needed to calculate the field of some juice
I hand out beers for everyone doing that!
Comment #34
alexpottComment #35
chx CreditAttribution: chx commentedEdit: nevermind.
Comment #36
alexpott$success = $this->value === $other;
from core/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.phpComment #37
dawehnerThank you!
Comment #38
chx CreditAttribution: chx commentedAlso? There's nothing before. And there's no verb.
Stores on what object? And this doesn't return anything. "@return void" seems to be the way.
An array of formulae? Most of the patch uses the latin plural of formula (which I do not like but if you do I won't object too much) and let's be consistent. The "for" is almost surely wrong unless my English grammar is badly off. When I (emphasis on I) see an "array for formulae" I expect a by reference array which gets populated.
Sure, we can do a doxygen todo, I have no problems except one: catch said "naked" todos are useless and all of them should be accompanied by an issue. Please file one.
locale.translation.formulae perhaps?
different services.
Comment #40
stefan.r CreditAttribution: stefan.r commentedAddressed #38
Formulas might be better, but not too bothered as this is only used on protected variables/methods and not part of the API.
Comment #43
stefan.r CreditAttribution: stefan.r commentedBy the way drush HEAD currently fails on
drush si
- I believe this patch would fix that?Comment #45
chx CreditAttribution: chx commentedYes, drush + HEAD fails https://github.com/drush-ops/drush/pull/1619 issue + crude fix but this is better. Apparently
if (isset($input[$element['#name']]) && $input[$element['#name']] == $element['#value']) {
and$input[$element['#name']]
becomest('Save and continue')
and then it crashes. So this is a better fix.Comment #49
alexpottComment #50
phenaproximaYAY! This fixed the fatal error I was getting when running drush si:
Fatal error: Nesting level too deep - recursive dependency? in /Users/phen/Sites/drupal8/core/lib/Drupal/Core/Form/FormBuilder.php on line 1310
Applying this patch corrects the issue. Looking at the code which triggered the fatal, the problem (and solution) make perfect sense. +1 RTBC.
Comment #51
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI also get the same fatal from Drush as #43/#45/#50, and also fixed with this patch, so yay!
I want to review this a bit more thoroughly before committing myself, but no objection to another committer beating me to it.
Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis looks great, and because it fixes a fatal error when installing Drupal via Drush, I pushed it to 8.0.x without a change record. But I think we do need a change record for this, both explaining the general decoupling and specifically mentioning the methods removed from interfaces:
Comment #54
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedSince this was commited I have some Exceptions in some modules like monitoring and simplenews, the exception says that the service added in this patch "locale.plural.formula" doesn't exist.
This is a problem in my modules or in the core due to this commit?
Comment #55
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedThis is the complete Exception:
This just hapens in local not in the testbot (I'm running it in PHP 5.6).
Comment #56
BerdirI think the problem is in PluralTranslatableString::getPluralIndex(). That checks the existence of the function, and that function then calls out to that service. But if the parent site has locale enabled then that module is loaded and there is nothing you can do about that.
Sounds like we need to change that to a module exists?
Comment #57
alexpottOpened #2573975: function_exists check in PluralTranslatableString is wrong
Comment #58
andypostThat needs follow-up
duplicates each other
formula(e)?
suppose 'formulas'
Comment #59
OleksiyFilled follow-up #2613976: Remove redundant "See..." that duplicates "@see..." in protected method documentation
Comment #60
YesCT CreditAttribution: YesCT commentedchange records can and should still be able to be updated, and since that is why this issue is still open, adding a tag so we can find this.
Comment #61
YesCT CreditAttribution: YesCT commentedadding link to instructions on how to draft a change record to the issue summary remaining tasks section
https://drupal.org/contributor-tasks/draft-change-record
Comment #63
plachPlease, can we get a CR for this?
Comment #68
vijaycs85Drafted a CR at https://www.drupal.org/node/2801819
Comment #69
Gábor Hojtsy@vijaycs85: this happened before 8.0.0, so the branch for 8.1.x was not correct. Also fixed a header problem: https://www.drupal.org/node/2801819/revisions/view/10067211/10070045
As @effulgentsia said though
that is not covered in the change entry yet.
Comment #70
vijaycs85Thanks @Gábor Hojtsy for the review. I have updated the CR to reflect @effulgentsia comment.
Comment #71
Gábor HojtsyReviewed, updated a bit. Looks all done now :) Moving back to the right version.
Comment #72
Gábor HojtsyAlso finally removing from the sprint.