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.Follow up to #1890502: WYSIWYG: Add CKEditor module to core.
Method names get*Plugins() imply that what is returned are plugins (i.e., either plugin definitions or instances). However, these methods currently return file names and button arrays. I think we should have these methods return the $plugin instances, and let calling code call getFile() or getButtons() as desired. But, if we want these methods returning what they currently return, then let's adjust their method names accordingly.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 1905018-10.patch | 11.39 KB | effulgentsia |
| #10 | interdiff.txt | 3.16 KB | effulgentsia |
| #9 | 1905018-9.patch | 12.87 KB | Wim Leers |
| #9 | interdiff.txt | 3.21 KB | Wim Leers |
| #5 | 1905018-5.patch | 11.02 KB | Wim Leers |
Comments
Comment #1
Wim LeersWhat about
get(Enabled|Buttons)PluginsMetadata()?Comment #2
Wim LeersIn this patch:
getEnabledPlugins()→getEnabledPluginFiles()getButtonsPlugins()→getButtons()Comment #3
Wim Leers#2: 1905018-2.patch queued for re-testing.
Comment #5
Wim LeersStraight reroll of #2. This one should pass tests.
Comment #6
Wim Leers#5: 1905018-5.patch queued for re-testing.
Comment #7
webchickHm. Not sure we can do this anymore now that we're post-API freeze? At least not without leaving the old versions in with a @deprecated tag, if I'm reading http://buytaert.net/drupal-8-api-freeze correctly.
Should we just postpone to D9?
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedSigh. My bad for not RTBCing #5 when it was posted, which was before the (timezone adjusted) API freeze deadline. I still think we should do this, but yeah, let's keep the old methods as wrappers to the new ones, and add a @deprecated to them.
I think it's a DX improvement to have (and within core, use) the new names. The D8 plugin system will be pretty foundational to many D8 contrib modules, so I'd like to minimize confusion that could arise from overloading the word with extraneous meanings.
Comment #9
Wim Leers#7/#8: I understand the rules, and I've followed them in this reroll. But, really … there's nobody out there who has implemented it yet. Plus, it's not even the plugin interface, it's the plugin manager, and that should only very rarely be used by any code outside the CKEditor module itself. Somebody having written an alternative
CKEditorPluginManageris even less plausible.So … do we really want to retain the old deprecated functions? :)
If the answer is as simple as "we don't want to start making exceptions", then that's fine. But this is a rather ridiculous way of renaming functions, of course :)
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedThis just removes redundant docs from the deprecated functions. Otherwise, RTBC.
I agree with #9 that there's probably no D8 module out there yet that's invoking the deprecated functions, so I leave it to a core committer to decide on whether to leave them deprecated (this patch), or remove them entirely (#5).
Comment #11
Wim Leers#10: okay, great, I already found it extremely silly to duplicate all docs, very happy to be wrong :)
Comment #12
webchickAll right, since this was ready before the API freeze deadline, I guess it can go in. Let's go with #5 then, because I agree that it's unlikely there are CKEditor plugin modules for D8 yet.
Committed and pushed to 8.x.
Comment #13
Wim Leers.