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
Follow-up to #2625258: LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object which added @internal methods and properties to not introduce any code breakages in 8.0.x
Proposed resolution
Rename the property $_configManager and make it protected. Replace _setConfigManager and _getConfigManager with constructor injection.
Remaining tasks
None
User interface changes
None
API changes
None
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff-18.txt | 803 bytes | joshi.rohit100 |
| #18 | 2628132-18.patch | 4.13 KB | joshi.rohit100 |
| #14 | interdiff-2628132-10-14.txt | 1.56 KB | id.medion |
| #14 | locale-ci-2628132-14.patch | 4.1 KB | id.medion |
Comments
Comment #2
alexpottThis should be postponed on #2625258: LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object
Comment #3
catchThat's now in.
Comment #4
alexpottComment #5
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedComment #6
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedDo we want to get rid of the getter entirely with this issue?
Comment #7
tstoeckler@jordanpagewhite yes, that is the idea. That's why it was specifically prefixed with _ and is also private.
Comment #8
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commented@tstoeckler Cool, thanks for the feedback. I gave it a shot in this patch. If I misunderstood the direction that we're shooting for, just let me know.
Comment #9
tstoeckler@jordanpagewhite That's perfect!.... except for one tiny little detail ;-) you forgot to update the
$config_entity_type = $this->_getConfigManager()->getEntityTypeIdByName($name);line to use$this->configManagerinstead.Can you post an updated patch, that would be great? You can set your patch to "Needs review" then, so it will get tested automatically.
Comment #10
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedHi jordanpagewhite/tstoeckler,
Thanks for the patch. You need to change the status to "Needs Review" for the patch to be tested.
@tstoeckler, I have added the change you asked for. Changed
$config_entity_type = $this->_getConfigManager()->getEntityTypeIdByName($name); to $config_entity_type = $this->configManage;
Is it what you have asked for? If not, kindly suggest what need to be changed.
Comment #12
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedPatch is working fine on my Local. Could anyone suggest the reason it failed the test. Thanks in advance.
Comment #13
catchThe service definition in core/services.yml needs updating too.
Comment #14
id.medionHi everyone,
I have updated the locale.services.yml according to changes in __construct(). Please review.
Comment #15
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedComment #17
claudiu.cristeaComment #18
joshi.rohit100Comment #19
claudiu.cristeaComment #21
Kristen PolThe patch looks good. What's the best way to test?
Comment #22
catch@Kristen Pol this is covered by existing automated testing, so doesn't need any manual testing at all.
Comment #23
Kristen Pol@catch Thanks for the quick reply. Good to know!
Comment #24
dawehnerNice!
Comment #25
alexpottCommitted d5cb79a and pushed to 8.1.x and 8.2.x. Thanks!
Comment #28
Gábor HojtsyThanks!