Problem/Motivation

When editing a ConfigEntity it loads the Entity in the language from language negotiation. But it actually should load the untranslated version, as ConfigEntities are translated via a separate Translation Tab.
This is how Configurations Work as well.

Proposed resolution

Load the untranslated Version when editing a ConfigEntity

Remaining tasks

Needs Tests

User interface changes

none

API changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +language-config

At least a major.

stefan.r’s picture

Assigned: Unassigned » stefan.r
tim.plunkett’s picture

Gábor Hojtsy’s picture

@tim.plunkett: yeah question is how fast would that be done. This bug was independently reported by several people this week as they try things out and attempt to help.

tim.plunkett’s picture

I have no problem with anyone trying to solve this, especially if its straightforward/easy. I just didn't want someone sinking a huge amount of time/effort and have it ripped out all of a sudden.

Let's talk tomorrow

Gábor Hojtsy’s picture

I'm not sure this would be ripped out. We'll need a solution for doing the entity upcasting with the proper "language context", and some kind of "language context" will change. It sounds like the API for that would change, but the logic is needed anyway.

Gábor Hojtsy’s picture

I'm not sure this would be ripped out. We'll need a solution for doing the entity upcasting with the proper "language context", and some kind of "language context" will change. It sounds like the API for that would change, but the logic is needed anyway.

Berdir’s picture

Oh, right, this already happens during param conversion, which answers my question in #2091871-1: Add an ConfigEntityForm with an exists() method.

Anonymous’s picture

yeah. i personally would like to see this postponed on #2098119: Replace config context system with baked-in locale support and single-event based overrides, if for no other reason than it will help focus multilingual folks on that issue.

that issue is at the top of my list, so as soon as it's ready, i'll be harrassing committers to get it in.

Gábor Hojtsy’s picture

Status: Active » Postponed

I understand that. I think the language switching logic will need to be here as well as the tests. It may use a slightly different API, but most of the patch would be the same at the end. *I* think parallelisation would be great, but we can postpone this if that helps focus efforts.

Schnitzel’s picture

Status: Postponed » Needs review
Issue tags: +Need tests
FileSize
2.17 KB

first version, works on my local, let's see how much testbot likes this, still needs tests

Schnitzel’s picture

Oh and an explanation why I think we should get this in, even with #2098119: Replace config context system with baked-in locale support and single-event based overrides this could be fixed:

  1. A lot of people are trying out D8 out now and they are super confused with that they destroy ConfigEntities with just saving them
  2. We don't know really how long #2098119: Replace config context system with baked-in locale support and single-event based overrides will take to get in
  3. #2098119: Replace config context system with baked-in locale support and single-event based overrides could fully fix this, but I think it's still good that locale Module is in charge of overwriting the logic that a ConfigEntity is loaded without language
  4. @beejeebus if you would like to have some help for #2098119: Replace config context system with baked-in locale support and single-event based overrides and this specific issue, just hit me
Gábor Hojtsy’s picture

@Schnitzel: I think it goes without question that #2098119: Replace config context system with baked-in locale support and single-event based overrides would love your help :)

Schnitzel’s picture

and here with Tests, one which should Fail with Current HEAD.

Status: Needs review » Needs work
stefan.r’s picture

This patch is basically the same as the previous patch other than some naming changes, comments and adding another permission in the test.

Thanks to webflo, fubhy and Schnitzel for helping me out with this issue :)

stefan.r’s picture

Status: Needs work » Needs review
FileSize
657 bytes
4.06 KB

I had accidently defined locale_config_subscriber twice in locale services file.

Revised patch attached.

fubhy’s picture

  1. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathEntityConverter.php
    @@ -0,0 +1,54 @@
    + * ¶
    

    There are some whitespace issues in this patch.

  2. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathEntityConverter.php
    @@ -0,0 +1,54 @@
    +class LocaleAdminPathEntityConverter extends EntityConverter implements ParamConverterInterface {
    

    No need to implement the interface explicitly. EntityConverter already does that (which you extend from here).

  3. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathEntityConverter.php
    @@ -0,0 +1,54 @@
    +      // Enter the override-free context, so we can ensure no overrides are applied.
    

    Exceeds the 80 character limit.

  4. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathEntityConverter.php
    @@ -0,0 +1,54 @@
    +  public function applies($definition, $name, Route $route) {
    +    if (parent::applies($definition, $name, $route)) {
    +      // path_is_admin() needs the path without the leading slash.
    +      $path = ltrim($route->getPath(), '/');
    +      return path_is_admin($path);
    +    }
    +    return FALSE;
    

    To clarify what this does:
    Due to this converter having a higher weight than the default EntityConverter every time this method returns TRUE it overrides/takes over this route from EntityConverter (we only allow a single converter per route argument). So as soon as this converter says "Yes, I'll convert this parameter" we ignore EntityConverter.

    What is not okay though is that you are currently overriding this for EVERY entity type in the backend (on an admin path) while you really only want to override it for config entities.

    Hence, you should check for whether the entity type implements ConfigEntityInterface right here in the applies() method.

Other than that, this seems okay.

fubhy’s picture

I hope I understand this right and you want to only override the upcasting behavior of config entities, right? If that is true than you should also consider renaming the class to LocaleAdminPathConfigEntityConverter

stefan.r’s picture

fubhy, thanks for the feedback and you did understand this correctly!

I attached the revised patch, which also includes further documentation in the applies() method.

(edit: the interdiff is against #16 not #17, disregard it.)

fubhy’s picture

Status: Needs review » Needs work

Thanks stefan.

  1. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -0,0 +1,67 @@
    + * Contains Drupal\locale\ParamConverter\LocaleAdminPathConfigEntityConverter.
    

    Misses leading backslash (\Drupal).

  2. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -0,0 +1,67 @@
    + * {@inheritdoc}
    + *
    + * Makes sure the untranslated entity is loaded on admin pages.
    ...
    +   * {@inheritdoc}
    +   *
    +   * Converts to the entity in the default language as opposed to converting to
    +   * the entity in the negotiated language.
    ...
    +   * {@inheritdoc}
    +   *
    +   * This converter applies only if the path is an admin path.
    +   *
    +   * Due to this converter having a higher weight than the default
    +   * EntityConverter, every time this method returns TRUE it takes over the
    +   * route from EntityConverter. As we only allow a single converter per route
    +   * argument, EntityConverter is ignored when this converter applies.
    

    Afaik we don't use {@inheritdoc} on classes. Also, we don't use {@inheritdoc} and then extend it. Either it's just {@inheritdoc} or it's just the custom description. (Unless our policy for that changed lately.)

  3. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -0,0 +1,67 @@
    +  ¶
    

    Whitespace.

  4. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -0,0 +1,67 @@
    +      $entity_type_info = entity_get_info($entity_type);
    +      $reflection = new \ReflectionClass($entity_type_info['class']);
    +      if ($reflection->implementsInterface('\Drupal\Core\Config\Entity\ConfigEntityInterface')) {
    

    No need to use reflection there. A simple "is_subclass_of" should be sufficient.

Note: When doing an interdiff on a patch where you moved/renamed a file it is better to use "git diff -M" which recognizes renames/moved files and thereby drastically reduces the size of the interdiff.

stefan.r’s picture

Latest try attached.

Re #2, I was told on IRC the problem is there's not really a policy :)

I'll go with just the custom description for now!

stefan.r’s picture

Status: Needs work » Needs review
fubhy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -0,0 +1,60 @@
    + ¶
    

    Whitespace (if you use an IDE you should configure it to remove those automatically or configure Git to warn you about those.

  2. +++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
    @@ -0,0 +1,60 @@
    +      $info = entity_get_info($entity_type);
    

    You should use $this->entityManager->getDefinition() instead of entity_get_info().

Other than that this looks good.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -sprint, -Need tests
FileSize
1.06 KB
4.68 KB

Set up my IDE to get rid of trailing whitespace, saving me valuable storage space :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks RTBC from my point of view, confirmed with @fubhy on IRC too :)

fubhy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
@@ -0,0 +1,60 @@
+   * Converts to the entity in the default language as opposed to converting to
+   * the entity in the negotiated language.
...
+   * This converter applies only if the path is an admin path.
+   *
+   * Due to this converter having a higher weight than the default
+   * EntityConverter, every time this method returns TRUE it takes over the
+   * route from EntityConverter. As we only allow a single converter per route
+   * argument, EntityConverter is ignored when this converter applies.

Here is my suggestion: Since we don't have a inheritdoc policy, please merge these docblocks into the class documentation and use a plain {@inheritdoc} on the methods instead. Becuase otherwise you have to re-document the arguments, etc. which sucks.

EDIT: But yes, RTBC otherwise :). So feel free to switch back to RTBC once the docblocks are fixed.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
4.77 KB

Rerolled with that.

fubhy’s picture

Assigned: stefan.r » Unassigned
Status: Needs review » Reviewed & tested by the community

This could've used a tiny unit test... But, oh well..

Gábor Hojtsy’s picture

It has a nice webtest though :) Which is pretty appropriate IMHO given we are affecting upcasting in the route handler, so doing a request and seeing what comes out sounds like a good way to test this to me.

Gábor Hojtsy’s picture

Issue tags: +sprint
Gábor Hojtsy’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

I think this is OK. The functional test will still be there for the context-removal issue. Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks, superb.

Schnitzel’s picture

Great! Thanks stegan.r for fixing and helping on this one :)

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +Configuration context
semiaddict’s picture

This seems to be causing some issues with sites on which the installed language is no longer the default language.
See https://www.drupal.org/project/drupal/issues/2896235.

semiaddict’s picture

It seems issue #2896235 is not related to the work done here after all.
The issue seems to be coming from \Drupal\language\Config\LanguageConfigFactoryOverride.
Sorry.