Problem/Motivation
This came up in #2546212: Entity view/form mode formatter/widget settings have no translation UI. We want to translate node displays on the node type translation form, but we cannot just attach *all* node displays to the mapper alltogether, but we need to dynamically attach those displays that belong to the node type that is currently being translated. And we cannot do this in a dedicated mapper, because we need to do this generically from Field UI module for all entity types.
I.e. when ConfigEntityMapper::setEntity()
is called for the node type mapper, it ends up setting the node.type.article
config name, for example. We need to somehow hook into that from Field UI and also attach core.entity_view_display.node.article.teaser
for example.
This is blocking a major bug, so is tagged as major as well.
Proposed resolution
Have config_translation module dispatch an event when populateFromRouteMatch() is called so that other modules can subscribe to the event and add config names to the config mapper.
Remaining tasks
Review.
Evaluate if we need the route match in the event. See this comment for a reason why we should.
User interface changes
None.
API changes
Additions:
- New event (ConfigMapperPopulateEvent)
- New argument (optional for BC) for ConfigNamesMapper & ConfigEntityMapper ($event_dispatcher)
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#73 | 2577761-73.patch | 18.47 KB | tstoeckler |
Comments
Comment #2
penyaskitoBlocker then
Comment #3
penyaskitoBlocker then
Comment #4
Gábor HojtsyAny suggested options? :)
Comment #5
vijaycs85Just looking at the config schema of core.entity_view_display.*.*.* (in core.entity.schema.yml) and I don't see any translatable elements there. Here is few more points to prove above statement.
1. 'label' under core.entity_view_display.*.*.*.content.[field_name] is a placement (hidden/above/inline) *string* not translatable *label*
2. field.formatter.settings.[%parent.type] is a dynamic definition under this schema and as far as core implementation, none of them have translatable element(s).
3. field.formatter.settings.[%parent.type] is another dynamic item and core has no implementation/example for this.
Standard profile provides list of entity_view_displays (below) out of box, but there isn't any translatable elements.
core.entity_view_display.block_content.basic.default
core.entity_view_display.comment.comment.default
core.entity_view_display.node.article.default
core.entity_view_display.node.article.rss
core.entity_view_display.node.article.teaser
core.entity_view_display.node.page.default
core.entity_view_display.node.page.teaser
core.entity_view_display.user.user.compact
core.entity_view_display.user.user.default
now, question #1. Am I missing a big picture here? #2. Are we worried about the 3rd party settings? #3. Do we have any working example with the issue to start with?
update: found admin/structure/display-modes/view/manage/node.full/translate/ta/add has just name as translatable.
Comment #6
tstoecklerSorry for not explaining the use-case earlier.
Yes, we are. :-) Fieldgroup module will want to add it's information there including the label of the fieldgroup, I presume. It would be very unfortunate to not be able to translate that. Also it's conceivable that widgets or formatters have translatable settings in contrib even if they don't in core.
Comment #7
tstoecklerSome ideas:
The easy way out here would be to just throw an event either in
ConfigNamesMapper::populateFromRouteMatch()
or inConfigEntityMapper::setEntity()
. Then Field UI module could react to that event and add its names to the mapper. The former would have the upside of not being tied toConfigEntityMapper
, i.e. if there were another dynamic use-case of the mappers then they could re-use that event. The latter would have the advantage that the implementor in Field UI module could work with the entity obejct directly instead of having to retrieve it from the route match, thus duplicating code fromConfigEntityMapper::populateFromRouteMatch()
.A cooler way to solve this would be to add config names with placeholders to the mappers and then have
ConfigNamesMapper::populateFromRouteMatch()
do the replacing automatically. I.e. addnode.type.{node_type}
to the node type mapper and then Field UI module could addcore.entity_view_display.node.{node_type}.teaser
(among others). I think that would be a much nicer implementation. Right now I think it's very weird that e.g. the node type mapper has no config names in it untilpopulateFromRouteMatch()
is called, so in that way it's not distinguishable from any other config entity mapper. But I think that would be a much larger change and include API changes (whereas the above would be strictly API additions) so I think that's off the table at this point.Thoughts?
Comment #8
tstoecklerActually on throwing an event in
ConfigNamesMapper::populateFromRouteMatch()
or inConfigEntityMapper::setEntity()
:With #2565031: Expose $entity in ConfigEntityMapper in we now have a semantic way to fetch the entity from the mapper without duplicated logic. So I don't see any arguments against
ConfigNamesMapper::populateFromRouteMatch()
. Will try that out if there aren't any other ideas.Comment #9
tstoecklerAlso on the "cooler" solution with placeholders in config names: The example above is slightly lying because we can't just add
core.entity_view_display.node.{node_type}.teaser
because we don't know if the teaser display exists explicitly for each node type. So what we would in fact is something likecore.entity_view_display.node.{node_type}.*
where the*
would signify that we would have to callConfigFactoryInterface::listAll()
inpopulateFromRequest()
or something like that. In any case, I don't think that's something we can get away with just now, maybe in 8.1 if we manage to avoid BC-breaks.Comment #10
tstoecklerSomething like this?
Comment #13
Gábor HojtsyThat looks fine with me. Modules can subscribe to that event and then examine the mapper class and contents for extending config names. Indeed needs tests.
Comment #14
maxocub CreditAttribution: maxocub commentedHere's a new patch trying to make phpunit tests pass.
Now I'll work on new test coverage.
Comment #15
maxocub CreditAttribution: maxocub commentedComment #16
maxocub CreditAttribution: maxocub commentedComment #18
tstoecklerAwesome work @maxocub, that is a great test. Not really sure why it is failing the way it is, the test looks pretty harmless to me...
In any case, while the test itself is great, I think we can improve one thing about it:
The test doesn't need any UI functionality in order to work - the only interaction with the system is fetching a service - so we should really be making it a
KernelTestBase
, preferably a\Drupal\KernelTests\KernelTestBase
(yes, there are twoKernelTestBase
classes in core...).In that case it should go into a
tests/src/Kernel
directory in theconfig_translation
module directory. There is already aUnit
directory there with some tests in it which you can use to see how the namespace has to be (that is somewhat unintuitive).I'm not sure how we should name the test (maybe something generic like
ConfigMapperTest
?), but if you extendKernelTestBase
and put a$modules = ['config_translation', 'config_translation_test']
at the top you should be able to copy your test method 1-to-1. The only thing I'm not sure about: I don't know if\Drupal::service()
works in aKernelTestBase
, if it doesn't you can use$this->container->get()
, that should definitely work.Comment #19
maxocub CreditAttribution: maxocub commentedThanks @tstoeckler, I didn't know about KernelTestBase, and I though it was weird to put this test with the UI tests, but I didn't know where else to put it. I'll make the changes.
The other tests are failing because of denied access to the translation pages, caused by config_translation_test adding a config name to the mapper. I don't know why, maybe you do?
Comment #20
tstoecklerAhh good catch. That's probably because no config exists with that name. It has to be
config_translation_test.content
because that's the actual config object.Presumably, the failures will go away, anyway, though, when we move this to a separate test.
Comment #21
maxocub CreditAttribution: maxocub commented@tstoekler:
1) I changed the config name that I add to 'config_translation_test.content' and it made most of the tests pass. There is still some that fail with a PHP error, something about only one language allowed by config mapper (?), I don't remember exactly, I'll verify when I'm back home.
2) I have troubles moving my test and use KernelTestBase. As you said, '\Drupal::service()' doesn't work, and '$this->container->get()' does, but it doesn't seem to return a ConfigMapperManager object (when i call getMappers() on it the test fails). And it's hard to debug because when I try to print something to the console, nothing get printed, so I can't have any clue about what is returned. Any inputs?
Comment #22
tstoeckler@maxocub, it's hard to understand what is happening without seeing the code. Can you post what you currently have? It doesn't matter if it's not finished yet.
Comment #23
maxocub CreditAttribution: maxocub commentedSure, I'll do that tonight.
Comment #24
maxocub CreditAttribution: maxocub commentedHere's what I have.
It fails when I add th line '$mappers = $manager->getMappers();'
Comment #25
maxocub CreditAttribution: maxocub commentedUpdate: \Drupal::service('plugin.manager.config_translation.mapper') does work in a KernelTestBase test, and it returns a ConfigMapperManager as expected. But still, when I call getMappers() on it, the test fails.
Comment #26
maxocub CreditAttribution: maxocub commentedHere's a patch with the new test changed for a KernelTestBase one.
Thanks @tstoeckler for your help debugging this.
Comment #28
maxocub CreditAttribution: maxocub commentedAbout the failing UI test, this is the fatal error I get when I run it on my computer:
Comment #29
maxocub CreditAttribution: maxocub commentedThis is what I used to subscribe to the event and add a config name to a mapper:
This adds the config name to all mappers, and I think thats why the test fails.
Here's a patch where I add the config name only to a specific mapper, like this:
Is that how a module could add a config name to one mapper?
Comment #31
maxocub CreditAttribution: maxocub commentedI think the error mentioned in #28 is caused by one of the test from ConfigTranslationUiTest (line 541) who changes the langcode of the system.site config to 'not-specified', while the config name I add in config_translation_test is in English.
I'll try to investigate more this weekend, but do you have any idea how to solve that @tstoeckler?
Comment #32
tstoeckler@maxocub: Ahh, nice find! In that case, let's just add a
$mapper->getLangcode() === 'en'
to the condition inConfigTranslationTestSubscriber
, that seems to be the easiest.Comment #33
maxocub CreditAttribution: maxocub commented@tstoeckler: Good idea, thanks! Let's see how it goes...
Comment #34
tstoecklerYay, this is now ready for some serious reviews. One thing that I thought about when working on #2546212: Entity view/form mode formatter/widget settings have no translation UI was whether we should also be providing the route match in the event. In the end did not require it in that issue, but it seems like it might be useful for generally. I am slightly leaning towards "Yes", but not sure.
If we do, we should rename the event. It's now generically named "ConfigMapperEvent" because all it knows about is a config mapper, but if it also knows about a route match we should name it after its specific purpose: "ConfigMapperPopulateEvent".
Comment #35
Gábor HojtsyThanks for the test coverage!
The issue definitely needs an issue summary update to outline the solution. For me its not clear for example which use cases would benefit from the route match passed around.
Also as an API change/addition, this would need to go to 8.1. I think it should also aim to be backwards compatible. Now it swaps the argument order around for example and does not have a fallback way to work when the new arguments are not passed to the mappers. Marking needs work for that.
Comment #36
Gábor HojtsyComment #37
maxocub CreditAttribution: maxocub commentedReally not sure what I'm doing here, I just put the arguments order as it was before, but I don't know yet what are the implications. Just want to see how the tests go.
Comment #38
maxocub CreditAttribution: maxocub commentedComment #40
tstoecklerIn terms of the argument order or added arguments:
If that's considered a BC-break we could do the following:
Instead of:
we could do:
As far as I'm aware that's 100% backwards-compatible, and that's also what we've done elsewhere before.
So marking needs work for that. That way this should be uncontroversial for 8.1.
Comment #41
maxocub CreditAttribution: maxocub commentedThanks @tstoeckler!
Here's a new patch, let's see if it pass.
Still need to update issue summary, will do soon.
Comment #42
tstoecklerYay, @maxocub++!!!
This looks pretty much ready to go to me.
One minor thing:
ConfigTranslationTestSubscriber
could use some docs, i.e. @file at the doc and a docblock for the class and inheritdoc / proper docblock for the methods.Comment #43
maxocub CreditAttribution: maxocub commented@tstoeckler: I will add the missing docs.
I was wondering if we really needed this change? parent::populateFromRouteMatch() already call setLangcode() and dispatchMapperEvent(). Is it because we need to call setEntity() before dispatchMapperEvent()?
Comment #44
maxocub CreditAttribution: maxocub commentedHere's the new patch with the missing docs.
Comment #45
tstoecklerDocs look good.
@maxocub re #43: Yes, exactly. The current code is a bit unfortunate but I couldn't figure out how do to it any better. We need to avoid dispatching the event twice and we also need to call
setEntity()
(andsetLangcode()
) before dispatching it so that event subscribers have all the info they need. Looking at it again now, I think we should be able to doright? Not sure...
Also, coming back to this now after quite a while, I do think we should pass the route match to the event. It's called
POPULATE_MAPPER
after all, and that "populate" comes from the method namepopulateFromRouteMatch()
, so not passing it along is not nice IMO. We should then rename the event class toConfigMapperPopulateEvent
because it would be confusing for a genericConfigMapperEvent
(the current class name) to have agetRouteMatch()
method. I still do not feel strongly, though, so if others disagree we can also leave it as is. Would just be unfortunate if we then realize that some contrib module, does actually want access to the full route match.Comment #46
maxocub CreditAttribution: maxocub commented@tstoeckler: I'm trying your suggestion about the route match to see what it brings, I'm asking myself:
Do we need the dispatchMapperEvent() function or can we move its code to populateFromRouteMatch?
Because we would need the $route_match to construct the new ConfigMapperPopulateEvent, which right now we don't in dispatchMapperEvent().
Or else, what would be the best way to pass the $route_match to dispatchMapperEvent()? Add it as an argument?
Comment #47
tstoecklerNo, we don't really need it. Feel free to remove it and/or reorganize the code in any way you think it should be.
Comment #48
maxocub CreditAttribution: maxocub commentedUpdated the issue summary.
Uploaded a new patch with the route match in the (renamed) event.
Comment #49
Kristen PolThanks! Some very minor nitpicks:
Wrap param to fit in 80 chars? Not sure that is best practice.
Same as above.
Remove empty space?
Period at end of sentence.
More than 80 chars.
Comment #50
Kristen PolI updated the patch based on feedback in #49. Note:
Comment #51
tstoecklerCode looks really nice now, thanks @maxocub and @Kristen Pol !!!
I think it's good that we make the event dispatcher optional in
ConfigNamesMapper
, but I think we should do the same inConfigEntityMapper
as well. The= NULL
is missing there.Comment #52
tstoecklerOops, sorry, it's there indeed. Don't know what I was hallucinating...
Looks perfect, then. Unfortunately, I can't RTBC.
Comment #54
rodrigoaguileraI'm interested in testing this so I did a simple reroll
Comment #56
Wim LeersLooks great. Mostly nits :)
Why change call order here?
EDIT: because
\Drupal\config_translation\ConfigNamesMapper::populateFromRouteMatch()
is now dispatching an event, and at that point we want all metadata to be set. Therefore we must first set metadata ($this->setEntity(…)
) and then call the parent method.This is technically a BC break. At minimum this needs a CR to indicate that any subclasses of
\Drupal\config_translation\ConfigNamesMapper
must update theirpopulateFromRouteMatch()
method to call the parent method last.This TODO was not actually addressed AFAICT.
EDIT: lol, no, it just was an todo that's no longer relevant :)
We do this to maintain BC, because this is a base class?
If so, let's add a
@todo
to make this parameter required in 9.0.0.Nit: we don't do
@file
docblocks anymore :)Nit:
$this->routeMatch
.Nit: s/population/populating/
s/mapper event/mapper populate event/
s/an event/the "config mapper populate" event/
Comment #57
Wim LeersComment #58
tstoecklerThank you for your review!
Comment #60
bojanz CreditAttribution: bojanz at Centarro commentedThis looks almost ready.
We need to remove this.
Is there a reason for this change? I don't see one (setLangcode() still only assigns the value to $this->langcode)
This might be nitpickish, but I'd love to see a sentence explaining a use case for this.
Something like "Allows modules to add related config for translation on a specific translation form."
Comment #61
andypostAddressed #60 review
Comment #62
bojanz CreditAttribution: bojanz at Centarro commentedThank you! This now looks ready.
Small nitpick that can be ignored, or addressed at commit:
I generally dislike comments that retell what trivial code is doing. We already know what's happening from seeing the dispatch call and the event constant. So I'd remove the comment.
Comment #63
larowlanOnly some minor nits from me, in addition to @bojanz's above.
In line with https://twitter.com/xjmdrupal/status/922090356077932544 - probably should be fixed beforehand
nit:do we need the intermediate variable here?
assertContains/assertNotContains is your friend :)
Comment #64
xjmFor #62 and #63.
Comment #65
andypostFixed #62 & #63
added visibility
Comment #66
borisson_The interdiff in #65 correctly solves the #62 and #63.
Comment #68
tacituseu CreditAttribution: tacituseu commentedUnrelated failure (#2919773: Fail to download drupal/coder because github.com/klausi/coder has gone missing).
Comment #69
larowlanThis is looking very good
We need a follow up issue for this - which should be referenced in the code. I.e the URL to the issue for the follow-up.
this change looks out of scope
thanks for working on this
Comment #70
larowlanAdding credit for reviewers that helped shape the patch.
Comment #72
tstoecklerNeeds work per #69
Comment #73
tstoecklerRe #69.1: I grepped for " ?: \Drupal::service(" and nowhere else where we have that pattern do we even add a todo, so I just removed it here, as well.
Fixed 2.
Comment #74
Kristen PolI reviewed the patch in #73 for style and consistency and didn't notice anything obvious. The interdiff confirms that point 2 of #69 was covered and, regarding point 1 in #69, the note in #73 explains why the todo was removed.
It looks like everyone's feedback has been incorporated. I would RTBC but I provided a patch way back when so probably shouldn't?
Comment #75
bojanz CreditAttribution: bojanz at Centarro commentedWe're definitely ready again.
Comment #77
tacituseu CreditAttribution: tacituseu commentedUnrelated failure (DST).
Comment #79
tacituseu CreditAttribution: tacituseu commentedUnrelated failure.
Comment #81
BerdirBack to RTBC. This poor issue has been RTBC since february :)
Comment #83
tacituseu CreditAttribution: tacituseu commentedHEAD problem, looks like bd67565 missed a spot.
Comment #85
tacituseu CreditAttribution: tacituseu commentedComment #88
larowlanFixed on commit
Committed ffc8ecd and pushed to 8.7.x
C/p as b4a02f8 and pushed to 8.6.x.
Published change record