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
Comment | File | Size | Author |
---|---|---|---|
#28 | drupal-2099541-ConfigEntities-should-not-load-28.patch | 4.77 KB | Gábor Hojtsy |
#28 | interdiff.txt | 2.06 KB | Gábor Hojtsy |
#25 | drupal-2099541-ConfigEntities-should-not-load-25.patch | 4.68 KB | stefan.r |
#25 | interdiff.txt | 1.06 KB | stefan.r |
#22 | drupal-2099541-ConfigEntities-should-not-load-22.patch | 4.66 KB | stefan.r |
Comments
Comment #1
Gábor HojtsyAt least a major.
Comment #2
stefan.r CreditAttribution: stefan.r commentedComment #3
tim.plunkettWouldn't #2098119: Replace config context system with baked-in locale support and single-event based overrides solve this?
Comment #4
Gábor Hojtsy@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.
Comment #5
tim.plunkettI 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
Comment #6
Gábor HojtsyI'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.
Comment #7
Gábor HojtsyI'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.
Comment #8
BerdirOh, right, this already happens during param conversion, which answers my question in #2091871-1: Add an ConfigEntityForm with an exists() method.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedyeah. 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.
Comment #10
Gábor HojtsyI 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.
Comment #11
Schnitzel CreditAttribution: Schnitzel commentedfirst version, works on my local, let's see how much testbot likes this, still needs tests
Comment #12
Schnitzel CreditAttribution: Schnitzel commentedOh 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:
Comment #13
Gábor Hojtsy@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 :)
Comment #14
Schnitzel CreditAttribution: Schnitzel commentedand here with Tests, one which should Fail with Current HEAD.
Comment #16
stefan.r CreditAttribution: stefan.r commentedThis 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 :)
Comment #17
stefan.r CreditAttribution: stefan.r commentedI had accidently defined locale_config_subscriber twice in locale services file.
Revised patch attached.
Comment #18
fubhy CreditAttribution: fubhy commentedThere are some whitespace issues in this patch.
No need to implement the interface explicitly. EntityConverter already does that (which you extend from here).
Exceeds the 80 character limit.
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.
Comment #19
fubhy CreditAttribution: fubhy commentedI 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
Comment #20
stefan.r CreditAttribution: stefan.r commentedfubhy, 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.)
Comment #21
fubhy CreditAttribution: fubhy commentedThanks stefan.
Misses leading backslash (\Drupal).
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.)
Whitespace.
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.
Comment #22
stefan.r CreditAttribution: stefan.r commentedLatest 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!
Comment #23
stefan.r CreditAttribution: stefan.r commentedComment #24
fubhy CreditAttribution: fubhy commentedWhitespace (if you use an IDE you should configure it to remove those automatically or configure Git to warn you about those.
You should use $this->entityManager->getDefinition() instead of entity_get_info().
Other than that this looks good.
Comment #25
stefan.r CreditAttribution: stefan.r commentedSet up my IDE to get rid of trailing whitespace, saving me valuable storage space :)
Comment #26
Gábor HojtsyLooks RTBC from my point of view, confirmed with @fubhy on IRC too :)
Comment #27
fubhy CreditAttribution: fubhy commentedHere 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.
Comment #28
Gábor HojtsyRerolled with that.
Comment #29
fubhy CreditAttribution: fubhy commentedThis could've used a tiny unit test... But, oh well..
Comment #30
Gábor HojtsyIt 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.
Comment #31
Gábor HojtsyComment #32
Gábor Hojtsy#28: drupal-2099541-ConfigEntities-should-not-load-28.patch queued for re-testing.
Comment #33
catchI think this is OK. The functional test will still be there for the context-removal issue. Committed/pushed to 8.x, thanks!
Comment #34
Gábor HojtsyThanks, superb.
Comment #35
Schnitzel CreditAttribution: Schnitzel commentedGreat! Thanks stegan.r for fixing and helping on this one :)
Comment #37
Gábor HojtsyComment #38
semiaddict CreditAttribution: semiaddict commentedThis 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.
Comment #39
semiaddict CreditAttribution: semiaddict commentedIt 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.