Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
We have hook_config_translation_group_info hook to include config group & entity group into config translation.
Proposed solution
Convert hook to new plugin system.
Comment | File | Size | Author |
---|---|---|---|
#29 | configmap-7.patch | 23.36 KB | Gábor Hojtsy |
#29 | interdiff.txt | 941 bytes | Gábor Hojtsy |
#28 | configmap-6.patch | 23.29 KB | Gábor Hojtsy |
#28 | interdiff.txt | 1.2 KB | Gábor Hojtsy |
#27 | configmap-5.patch | 23.19 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyAFAIS there is no good way to let the information altered outside of an alter hook, so I think we should keep at least an alter hook, but the main definition would indeed be more standard as D8 plugins. See eg. https://api.drupal.org/api/drupal/core%21includes%21entity.api.php/funct... for altering information that comes from entity class annotations.
Comment #2
vijaycs85Adding incomplete patch to make sure we are going in right direction. Thansk @Gábor Hojtsy in advance.
Comment #4
Gábor HojtsyGood stuff!
Is this all it takes to make the ConfigEntityMapper available as an annotation? Wow!
filter?
not block :)
Comment #5
vijaycs85Adding few more plugins. Still needs plugin manager to make this work.
Comment #6
Gábor HojtsyTalked to EclipseGC about this. Summary:
- we better move this to a .yml file based discovery like #2065571: Add YAML Plugin discovery, tabs are going there too (#2050919: Replace local task plugin discovery with YamlDiscovery)
- add a manager then
The YAML based discovery is RTBC and close to commit.
Comment #7
EclipseGc CreditAttribution: EclipseGc commentedLooking at what you want to do here, you've kind of conflated the annotation and the plugin class. This was an easy mistake to make since you need to pass everything through the same class ConfigEntityMapper, but it's actually your plugin class, not your Annotation. In cases like these (where 99% of the plugin implementations go through the same class) using a discovery that is not class based makes better sense. I noticed you don't have a plugin manager yet, so I went to the effort of building a basic one that should get you close to working.
Eclipse
Comment #8
Gábor HojtsyYay #2065571: Add YAML Plugin discovery is in, so we can do this with .yml files now :)
@EclipseGC thanks for the starting manager code.
Comment #9
Gábor HojtsyHere is the code from @EclipseGC in patch form, plus I added the directory discovery lifted from Drupal\layout\Plugin\Type\LayoutManager. This may not be the very latest best practice, but list_themes() has no nice OO equivalent that I know for example. Not sure if this even works, I did not actually try :D
Comment #10
Gábor HojtsyAlso this only works for entity mappers obviously. Should we have yet another manager for the non-entity mapping? Not sure how will this end up with good DX... I think ideally we'd have one .yml file structure with possibly entity and config file mapping.
Comment #11
vijaycs85#2078405: Revert YamlDiscovery if it doesn't make sense as first-class discovery suggesting not to use YamlDiscovery for anything other than menu_type?
Comment #12
vijaycs85updating tag.
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedLet's not get too hung up on that issue. I'll wade in there and see if I can make sense of the situation. :-D
Eclipse
Comment #14
vijaycs85Thanks @EclipseGc. Here is the patch that gets the entity mapper from MODULE.entity.config.mapper.yml.
TODO:
1. Move dynamic entity mappers e.g. fields out of hook.
2. Move entity group out of hook.
Easiest way for now is to update config_translation_config_translation_group_info to config_translation_config_translation_group_info_alter and keep #1 and #2 in it. @Gábor Hojtsy?
Comment #16
vijaycs85Missed name in a location, here you go testBot...
Comment #17
Gábor HojtsyGreat, I committed this first step. I think it would be great to explore how to further improve this. I think its not very nice that we have a hook and a .yml file to say the least :D Eg. rename the *.entity.config.yml to something more generic and include the simple .yml mappings as well (eg. add a key to the data say 'type: entity' or 'type: config'. Then the dynamic entity based field mappings can go to an alter hook, which we would still keep. Moving to active since the patch was committed but still stuff to be done. Thanks all!
Comment #18
Gábor HojtsyHere is a proof of concept to add a type key to this .yml (renamed it to *.configmap.yml to signify the mode general purpose) and add name based maps as well.
Comment #20
Gábor HojtsyNeed to update the service map.
Comment #22
Gábor HojtsyOf course need to update the route subscriber as well.
Comment #24
Gábor HojtsyTest fails show the menu types are not getting over properly :)
Comment #26
Gábor Hojtsy- Removed the old groups hook and support for gathering data with that
- Renamed the file (again) to *.config_translation.yml as per discussion with @vijaycs85 on IRC
- The alter hook is config_translation_info_alter(), IMHO makes lots of sense (trying to get rid of the one-off groups terminology as much as possible)
- Updated docs
- hook_menu() was using the old hook, not the plugin manager, therefore the issues with tabs not showing up, fixed
- Re-introduced default definition elements so not all keys need to be provided in the .yml file; we can maybe default the type key, but I'm not sure about that one
- Fixed the RSS publishing fail by using a separate key :D
- Moved the field entity mapping stuff into the new alter hook which is now dependent on plugin definitions
This now passes all tests for me. Let's see here.
Comment #27
Gábor HojtsyIt helps if I post the right patch. The interdiff was correct, the patch was not.
Comment #28
Gábor HojtsyUpdated some docs as well to ensure up-to-dateness.
Comment #29
Gábor HojtsyFixed dots at the end of @see's (as per YesCT) and updated alter hook main line. Committing this one. I consider this done for this patch/issue. I'll continue cleaning up the plugin stuff in another issue.
Comment #30
YesCT CreditAttribution: YesCT commentedReading through this. Looks good.
tiniest of nits, https://drupal.org/node/1354#see no period in the end of @see (@Gábor Hojtsy says can fix before commit)
Not blocking, just noting: to reduce the info contrib (or otherwhere in core) has to specify things, like in config_translation.config_translation.yml, base_path: 'admin/structure/block/manage/{block}', config entites would have to be refactored to have a URI pattern, maybe in an annotation. Would have to be a core issue to do that. @effulgentsia might have thoughts.
Comment #31
YesCT CreditAttribution: YesCT commentedoops. crosspost.
Comment #32
vijaycs85nice work @Gábor Hojtsy.
Comment #33.0
(not verified) CreditAttribution: commentedUpdated issue summary.