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

Comments

alexpott created an issue. See original summary.

alexpott’s picture

catch’s picture

Status: Postponed » Active

That's now in.

alexpott’s picture

Issue tags: +Novice
jordanpagewhite’s picture

Assigned: Unassigned » jordanpagewhite
jordanpagewhite’s picture

Do we want to get rid of the getter entirely with this issue?

tstoeckler’s picture

@jordanpagewhite yes, that is the idea. That's why it was specifically prefixed with _ and is also private.

jordanpagewhite’s picture

FileSize
2.64 KB

@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.

tstoeckler’s picture

Status: Active » Needs work

@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->configManager instead.

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.

anil280988’s picture

Hi 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.

Status: Needs review » Needs work

The last submitted patch, 10: 2628132-10.patch, failed testing.

anil280988’s picture

Patch is working fine on my Local. Could anyone suggest the reason it failed the test. Thanks in advance.

catch’s picture

The service definition in core/services.yml needs updating too.

id.medion’s picture

Hi everyone,
I have updated the locale.services.yml according to changes in __construct(). Please review.

jordanpagewhite’s picture

Assigned: jordanpagewhite » Unassigned

Status: Needs review » Needs work

The last submitted patch, 14: locale-ci-2628132-14.patch, failed testing.

claudiu.cristea’s picture

Issue tags: +#SprintLUX2016, +SprintWeekend2016
joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
4.13 KB
803 bytes
claudiu.cristea’s picture

Issue tags: -#SprintLUX2016, -SprintWeekend2016

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Kristen Pol’s picture

The patch looks good. What's the best way to test?

catch’s picture

@Kristen Pol this is covered by existing automated testing, so doesn't need any manual testing at all.

Kristen Pol’s picture

@catch Thanks for the quick reply. Good to know!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d5cb79a and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed a120fed on 8.2.x
    Issue #2628132 by joshi.rohit100, id.medion, jordanpagewhite, anil280988...

  • alexpott committed d5cb79a on 8.1.x
    Issue #2628132 by joshi.rohit100, id.medion, jordanpagewhite, anil280988...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.