ckeditor5_module_implements_alter() is a very complex workaround due to its hook needing to account for two different other hooks that are actually different hooks but called at the same time due to ModuleHandler::alter()'s capability to invoke multiple alter hooks at once.
I think it simplifies things if we let Media check for ckeditor5 now, both are now stable modules, there isn't really any preference in either direction. And the logic is easier to understand if media conditionally adds the validation instead of adding it and then ckeditor5 removes it again.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !11167
Comments
Comment #3
berdirmedia module does now rely on the implicit default hook order based on module name. It also is completely unclear to me why we implement this twice instead of using the base form id like editor and ckeditor5, but wasn't sure about the scope here.
Comment #4
nicxvan commentedI'd like to request this be postponed on #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter
We are currently using that in examples and it's the only implementation of that grouping in core.
We do have test coverage for it, but removing all of the examples and changing how hmia works before will make that issue harder to understand this part of the implementation.
It will also be a lot easier to incorporate this issue after #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter vs rebasing #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter on top of this change.
Comment #5
smustgrave commentedIf no objections to #4
Comment #6
berdirRebased, obviously conflicted heavily, still a useful simplification.
Comment #7
smustgrave commentedLooks like a good clean up to me!
Comment #10
nod_+1 to the simplification but it seems like we're loosing some informations as to why it's not necessary when using ckeditor. Can we get more of the comment ported to media in addition to the logic?
Comment #11
berdirExtended the comment a bit. Feels a bit redundant to add it twice, but also overkill to refer from one to the other.
Comment #12
smustgrave commentedFeedback for a comment appears to be addressed
Comment #15
nod_Committed and pushed f7e7f3efe42 to 11.x and c4340b27c61 to 11.3.x. Thanks!