Move QuickEditImageController from image to quickedit

There doesn't seem to be any references in PHP code to the image.info or image.upload routes that use this controller. I checked where these routes came from, see #2421427: Improve the UX of Quick Editing single-valued image fields. The path to the routes is referenced in image.js which appears to be entirely quickedit related, so hopefully an easy move. There are a bunch of related styles as well.

Rough initial commit. Added some todo items, in code and in the MR. Not sure about deprecating the route or the library either.

Added draft CR

The D10 branch looks good to me! Just reviewed and there's nothing to add, the D9 branch I just added some minor comments. About the CR, do you think we need to add anything related to the JS files? I mean the library has been moved into the quickedit module, is it something that can affect the developers?

Changing parent to the issue tracking the steps to deprecate quickedit.

With the library changing namespace..

If modules have an explicit dependency on the library, I think we should probably leave the old library in image module + deprecated and unused in 9.4.x. This would trigger a deprecation message for an un-updated stable equivalent in contrib.

If modules/themes are altering the library via hook_library_info_alter() / library overrides, then just a change record is fine because we explicitly tell people that what gets passed to alter hooks is subject to change in between minor releases. If we know of specific modules it's likely to break, then nice to also open an issue against them.

Not assuming there isn't more to do here, but providing patches since the commits are frequent enough today that I wanted to provide an option that wasn't saying "unable to apply" within moments of its addition.

My review of the patch file for D9.4:

1. +++ b/core/modules/image/image.libraries.yml
   @@ -19,3 +19,4 @@ quickedit.inPlaceEditor.image:
   +  deprecated: The image/quickedit.inPlaceEditor.image asset library is deprecated in Drupal 9.4.0 and will be removed in Drupal 10.0.0. Instead use quickedit/quickedit.inPlaceEditor.image. See https://www.drupal.org/node/3280366
   

   I think we are missing a deprecation message test for this deprecation.

2. +++ b/core/modules/image/image.routing.yml
   @@ -71,29 +71,3 @@ image.effect_edit_form:
   -image.upload:
   -  path: '/quickedit/image/upload/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
   -  defaults:
   -    _controller: '\Drupal\image\Controller\QuickEditImageController::upload'
   -  options:
   -    parameters:
   -      entity:
   -        type: entity:{entity_type}
   -  requirements:
   -    _permission: 'access in-place editing'
   -    _access_quickedit_entity_field: 'TRUE'
   -    _method: 'POST'
   -
   -image.info:
   -  path: '/quickedit/image/info/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
   -  defaults:
   -    _controller: '\Drupal\image\Controller\QuickEditImageController::getInfo'
   -  options:
   -    parameters:
   -      entity:
   -        type: entity:{entity_type}
   -  requirements:
   -    _permission: 'access in-place editing'
   -    _access_quickedit_entity_field: 'TRUE'
   -    _method: 'GET'
   

   Can we remove these 2 routes in 9.4? If somebody is using them in contrib or custom, than there site will break if we remove the routes.

3. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -108,65 +104,15 @@ public static function create(ContainerInterface $container) {
   +    $controller = new QuickEditImageController(
   

   I am very sure that this does not work. Which QuickEditImageController class are we using here?

4. +++ b/core/modules/quickedit/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,264 @@
   +  public function __construct(
   +    RendererInterface $renderer,
   +    ImageFactory $image_factory,
   +    PrivateTempStoreFactory $temp_store_factory,
   +    EntityDisplayRepositoryInterface $entity_display_repository,
   +    FileSystemInterface $file_system
   +  ) {
   

   This is not how we style this in Drupal core. Please put it on a single line. If you would like to change this then please create a followup issue for it.

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTestBase.php
   @@ -45,7 +45,10 @@ abstract class StableLibraryOverrideTestBase extends KernelTestBase {
   diff --git a/core/themes/stable/css/image/editors/image.css b/core/themes/stable/css/image/editors/image.css
   
   diff --git a/core/themes/stable/css/image/editors/image.css b/core/themes/stable/css/image/editors/image.css
   deleted file mode 100644
   
   +++ /dev/null
   @@ -1,100 +0,0 @@
   diff --git a/core/themes/stable/css/quickedit/editors/image.css b/core/themes/stable/css/quickedit/editors/image.css
   
   diff --git a/core/themes/stable/css/quickedit/editors/image.css b/core/themes/stable/css/quickedit/editors/image.css
   new file mode 100644
   

   Just a question: I think a git rename would be better here. Less to review and less that could go wrong.
   @bnjmnm: Do you have in the file .gitconfig, the setting:

   [diff]
     renames = copies
     renameLimit = 7000
   

   Renaming in the patch file makes it a much smaller file.

+++ b/core/modules/image/image.libraries.yml
@@ -19,3 +19,4 @@ quickedit.inPlaceEditor.image:
+ deprecated: The image/quickedit.inPlaceEditor.image asset library is deprecated in Drupal 9.4.0 and will be removed in Drupal 10.0.0. Instead use quickedit/quickedit.inPlaceEditor.image. See https://www.drupal.org/node/3280366
I think we are missing a deprecation message test for this deprecation.

I don't think we need a test for this - we have generic test for the libraries deprecation API, which this uses.

Can we remove these 2 routes in 9.4? If somebody is using them in contrib or custom, than there site will break if we remove the routes.

Routes count as data structure/@internal, so they're generally fair game for removing/changing in a minor, although in practice we don't for high traffic routes like node add and similar. On the other hand it would not hurt to leave them in duplicated in 9.4

Adding patch for convenience. Interdiff is basically this commit.

RE #15 1-2 Looks like catch is OK with them as-is
RE #15 3-4 Good call, in general it looks like there's quite a few changes happening in a class that is getting deprecated in this very issue, and it seems like the chances this class even being used after this leans much closer to theoretical than possible. I reverted pretty much everything outside of adding the deprecation. It looks like these changes were made based on this feedback in the MR, which are clever suggestions, but it seems like this ultimately adds more overhead to getting this completed that isn't required by the scope of this issue.

No changes to the 10 patch, so the one in #14 can be considered active.

1. +++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
   @@ -9,11 +9,25 @@
   + *   no_ui = TRUE
   

   🤔 What does this do? AFAICT that's a thing for @FieldType plugins only.

2. +++ b/core/modules/quickedit/css/editors/image.css
   @@ -1,6 +1,6 @@
   + * Functional styles for the QuickEdit module's in-place editor.
   
   +++ b/core/modules/quickedit/css/editors/image.theme.css
   @@ -1,6 +1,6 @@
   + * Theme styles for the QuickEdit module's in-place editor.
   
   +++ b/core/modules/quickedit/js/editors/image.es6.js
   @@ -0,0 +1,375 @@
   +       * {@inheritdoc}
   
   +++ b/core/themes/stable/css/quickedit/editors/image.css
   @@ -1,6 +1,6 @@
   + * Functional styles for the QuickEdit module's in-place editor.
   
   +++ b/core/themes/stable/css/quickedit/editors/image.theme.css
   @@ -1,6 +1,6 @@
   + * Theme styles for the QuickEdit module's in-place editor.
   
   +++ b/core/themes/stable9/css/quickedit/editors/image.css
   @@ -1,6 +1,6 @@
   + * Functional styles for the QuickEdit module's in-place editor.
   
   +++ b/core/themes/stable9/css/quickedit/editors/image.theme.css
   @@ -1,6 +1,6 @@
   + * Theme styles for the QuickEdit module's in-place editor.
   

   Nit: Quick Edit module's Image in-place editor

   (Because it used to be a sole in-place editor in the Image module, now it's a specific in-place editor in the Quick Edit module.)

3. +++ b/core/modules/quickedit/js/theme.es6.js
   @@ -182,4 +182,90 @@
   +   * Theme function for the dropzone element of the Image module's in-place
   +   * editor.
   ...
   +   * Theme function for the toolbar of the Image module's in-place editor.
   

   🐛 Needs to be updated now 😅

4. +++ b/core/modules/quickedit/src/Controller/QuickEditImageController.php
   @@ -71,7 +71,13 @@ class QuickEditImageController extends ControllerBase {
   -  public function __construct(RendererInterface $renderer, ImageFactory $image_factory, PrivateTempStoreFactory $temp_store_factory, EntityDisplayRepositoryInterface $entity_display_repository, FileSystemInterface $file_system) {
   +  public function __construct(
   +    RendererInterface $renderer,
   +    ImageFactory $image_factory,
   +    PrivateTempStoreFactory $temp_store_factory,
   +    EntityDisplayRepositoryInterface $entity_display_repository,
   +    FileSystemInterface $file_system
   +  ) {
   

   🤔 Why reformat this? Isn't the current formatting in line with what core does?

5. +++ b/core/modules/quickedit/src/Controller/QuickEditImageController.php
   @@ -107,7 +113,7 @@ public static function create(ContainerInterface $container) {
   -  public function upload(EntityInterface $entity, $field_name, $langcode, $view_mode_id) {
   +  public function upload(EntityInterface $entity, string $field_name, string $langcode, string $view_mode_id) {
   

   🤔 Why change the signature?

6. +++ b/core/modules/quickedit/src/Controller/QuickEditImageController.php
   @@ -115,12 +121,18 @@ public function upload(EntityInterface $entity, $field_name, $langcode, $view_mo
   -      $field_validators['file_validate_image_resolution'] = [$field_settings['max_resolution'], $field_settings['min_resolution']];
   +      $field_validators['file_validate_image_resolution'] = [
   +        $field_settings['max_resolution'],
   +        $field_settings['min_resolution'],
   +      ];
   ...
   -      return new JsonResponse(['main_error' => $this->t('The destination directory could not be created.'), 'errors' => '']);
   +      return new JsonResponse([
   +        'main_error' => $this->t('The destination directory could not be created.'),
   +        'errors' => '',
   +      ]);
   
   @@ -165,7 +177,10 @@ public function upload(EntityInterface $entity, $field_name, $langcode, $view_mo
   -      return new JsonResponse(['errors' => $this->renderer->render($messages), 'main_error' => $this->t('The image failed validation.')]);
   +      return new JsonResponse([
   +        'errors' => $this->renderer->render($messages),
   +        'main_error' => $this->t('The image failed validation.'),
   +      ]);
   

   🤔 Why reflow these? Seems like a nice-to-have out-of-scope change?

Addresses #19 as well as addressing image assets that were not copied over. I did a round of comparing in hopes of trying to undo any additional out of scope changes and hopefully they're all addressed now.

Only one remark remains, then this will be perfect!

+++ b/core/modules/quickedit/css/editors/image.css
@@ -1,6 +1,6 @@
+ * Functional styles for the Quick Edit image field.

+++ b/core/modules/quickedit/css/editors/image.theme.css
@@ -1,6 +1,6 @@
+ * Theme styles for the Quick Edit image field.

+++ b/core/themes/stable/css/quickedit/editors/image.css
@@ -1,6 +1,6 @@
+ * Functional styles for the Quick Edit image field.

+++ b/core/themes/stable/css/quickedit/editors/image.theme.css
@@ -1,6 +1,6 @@
+ * Theme styles for the Quick Edit image field.

+++ b/core/themes/stable9/css/quickedit/editors/image.css
@@ -1,6 +1,6 @@
+ * Functional styles for the Quick Edit image field.

+++ b/core/themes/stable9/css/quickedit/editors/image.theme.css
@@ -1,6 +1,6 @@
+ * Theme styles for the Quick Edit image field.

Sorry, but: s/field/in-place editor/ 🙈

Addressing #23 and updating the 10x patch to incorporate the feedback provided to the 9x patch that was also applicable to it.

10.0.x tests failed only on Nightwatch toolbar tests, which are 100% unrelated. And … it's the same kind of failure we see on PHP 7.3 environments with Drupal 9.4/9.5, which #3281863: Nightwatch tests failing >50% of test runs on PHP 7.3 in 9.4.x and 9.5.x, as well as PHP 8.1 on 10.0.x is fixing.

Re-testing.

We probably should update the CR to mention that we are no longer loading image/quickedit.inPlaceEditor.image. We should also document the code change we expect people extending that library to make.

We also need a draft release note.

I'm wondering if there would be benefit in updating image/quickedit.inPlaceEditor.image to reference the files from Quickedit? That way there would not be as much of a risk of loading the same file twice, and we wouldn't have to keep the files in sync.

For #27.

Implemented #27.

Updated CR and added release note.

I did not do any changes since the last RTBC, and all those changes appear to sufficiently bring this back to RTBC worthiness.

Committed/pushed to 10.0.x, then the respective patch to 9.5.x and cherry-picked to 9.4.x, thanks!