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

Files: 
CommentFileSizeAuthor
#28 drupal-2099541-ConfigEntities-should-not-load-28.patch4.77 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,860 pass(es).
[ View ]
#28 interdiff.txt2.06 KBGábor Hojtsy
#25 drupal-2099541-ConfigEntities-should-not-load-25.patch4.68 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]
#25 interdiff.txt1.06 KBstefan.r
#22 drupal-2099541-ConfigEntities-should-not-load-22.patch4.66 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 58,902 pass(es).
[ View ]
#22 interdiff.txt2.06 KBstefan.r
#20 interdiff.txt5.19 KBstefan.r
#20 drupal-2099541-ConfigEntities-should-not-load-20.patch4.81 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 58,431 pass(es).
[ View ]
#17 drupal-2099541-ConfigEntities-should-not-load-17.patch4.06 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 58,912 pass(es).
[ View ]
#17 interdiff.txt657 bytesstefan.r
#16 drupal-2099541-ConfigEntities-should-not-load-16.patch4.24 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 58,919 pass(es).
[ View ]
#14 drupal-2099541-dont-load-configentity-language-14.patch2.95 KBSchnitzel
FAILED: [[SimpleTest]]: [MySQL] 59,126 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 drupal-2099541-dont-load-configentity-language-14-tests-only.patch797 bytesSchnitzel
FAILED: [[SimpleTest]]: [MySQL] 59,435 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 drupal-2099541-dont-load-configentity-language.patch2.17 KBSchnitzel
PASSED: [[SimpleTest]]: [MySQL] 59,417 pass(es).
[ View ]

Comments

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

At least a major.

Assigned:Unassigned» stefan.r

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

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

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.

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.

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

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.

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.

Status:Postponed» Needs review
Issue tags:+Need tests
StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,417 pass(es).
[ View ]

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

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

@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 :)

StatusFileSize
new797 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,435 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.95 KB
FAILED: [[SimpleTest]]: [MySQL] 59,126 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal-2099541-dont-load-configentity-language-14-tests-only.patch, failed testing.

StatusFileSize
new4.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,919 pass(es).
[ View ]

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 :)

Status:Needs work» Needs review
StatusFileSize
new657 bytes
new4.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,912 pass(es).
[ View ]

I had accidently defined locale_config_subscriber twice in locale services file.

Revised patch attached.

  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.

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

StatusFileSize
new4.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,431 pass(es).
[ View ]
new5.19 KB

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

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.

StatusFileSize
new2.06 KB
new4.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,902 pass(es).
[ View ]

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!

Status:Needs work» Needs review

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.

Status:Needs work» Needs review
Issue tags:-sprint, -Need tests
StatusFileSize
new1.06 KB
new4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.06 KB
new4.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,860 pass(es).
[ View ]

Rerolled with that.

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

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

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.

Issue tags:+sprint

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!

Issue tags:-sprint

Thanks, superb.

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.

Issue summary:View changes
Issue tags:+Configuration context