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.
drupal_container() is deprecated, and all calls in the ckeditor module need to be replaced with Drupal::service(), except for where the module_handler service is requested, which needs to be replaced with Drupal::moduleHandler() (see #1957154)
Comments
Comment #1
ebeyrent CreditAttribution: ebeyrent commentedComment #3
ddrozdik CreditAttribution: ddrozdik commentedAttached new patch with some modifications.
Comment #4
podarok#3 clean conversion
RTBC
Comment #5
ebeyrent CreditAttribution: ebeyrent commentedIs it confusing for developers new to Drupal 8 to see $this->container->get() vs. Drupal::service()?
Comment #6
ddrozdik CreditAttribution: ddrozdik commentedebeyrent, No, we should use $this->container in tests instead Drupal::service(), and it's ok, because container already available from parent class and don't need to call again it, just look at parent clases.
Comment #7
alexpottNeeds a reroll
Comment #8
pwieck CreditAttribution: pwieck commentedRe-rolled
Comment #10
pwieck CreditAttribution: pwieck commented#8: ckeditor-module-replace-drupal_container-2003430-8.patch queued for re-testing.
Comment #11
pwieck CreditAttribution: pwieck commentedBad testbot...Bad! Don't know why this failed so bad the first time. I guess submitting a test in the middle of the night is a not a good idea.
Comment #12
pwieck CreditAttribution: pwieck commentedckeditor seems to work as it should without errors on this mornings build.
Comment #13
alexpottNow that #1903346: Establish a new DefaultPluginManager to encapsulate best practices has landed lets do the inject properly here...
This means that CKEditorPluginManager will need to be converted to the new DefaultPluginManager class and then the CKEditor plugin will need to be converted to implement ContainerFactoryPluginBase.
This is because replacing drupal_container for \Drupal::service is just replacing a static with a static...
Comment #14
Wim LeersUpdating title.
Are you still up for it, DmitryDrozdik or pwieck? I promise fast reviews if you still are! :)
Comment #15
Wim LeersFix typo.
Comment #16
Wim Leers#13:
DefaultPluginManager
in #2039425: Convert CKEditorPluginManager to extend DefaultPluginManager.\Drupal\ckeditor\Plugin\Editor\CKEditor
is implementingContainerFactoryPluginInterface
as of #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms.ContainerFactoryPluginInterface
, since none of them use anything in the container.Hence the scope of this issue has been reduced to just removing the leftover
drupal_container()
calls in CKEditor's tests.Patch attached for just that.
Comment #18
Wim LeersComment #19
ebeyrent CreditAttribution: ebeyrent commentedLooks good.
Comment #20
Wim Leers.
Comment #21
webchickLooks pretty straight-forward.
Committed and pushed to 8.x. Thanks!
Comment #22
Wim Leers.
Comment #23.0
(not verified) CreditAttribution: commentedUpdated issue summary.