\Drupal\editor\Ajax\EditorDialogSave::__construct() should typehint as an array instead of a string

Another "How do we typehint with BC?" issue. We're not able to make the typehint change in 9.0 at this point, and we need to figure out how we can add typehints with the continuous upgrade path.

Patch #5 cant be applied in 9.2.x.Need reroll.

Checking patch core/modules/editor/src/Ajax/EditorDialogSave.php...
error: while searching for:
  /**
   * An array of values that will be passed back to the editor by the dialog.
   *
   * @var string
   */
  protected $values;

  /**
   * Constructs a EditorDialogSave object.
   *
   * @param string $values
   *   The values that should be passed to the form constructor in Drupal.
   */
  public function __construct($values) {
    $this->values = $values;
  }


error: patch failed: core/modules/editor/src/Ajax/EditorDialogSave.php:15
error: core/modules/editor/src/Ajax/EditorDialogSave.php: patch does not apply

Rerolled patch #5 for 9.2.x.

Makes sense to change, I agree with comment #8.

Sending D8 backport.

Restoring status after HEAD was broken.

In terms of typehinting, https://3v4l.org/h9cYK shows that this doesn't break child implementations.

However, it might break calling code.

http://grep.xnddx.ru/search?text=new+EditorDialogSave&filename= shows there are some places where the return is not an array

http://grep.xnddx.ru/node/31368503 and http://grep.xnddx.ru/node/31370333 are two examples

So I think perhaps we should change the phpdoc only in D9, and then in D10, add a typehint.

Which means we need to trigger_error for a deprecation for anyone calling the constructor where $values is not an array.

Needs work for those changes

Crediting myself for trawling through 3 pages of contrib grep to find those two cases 👩‍💻

Crediting myself for trawling through 3 pages of contrib grep to find those two cases

Totally deserved!

Sending comment only patch.

Seems simple and makes sense after reading the ticket.

#19 looks OK for 9.5 at this point, however per #17 we need a (now 10.1.x) patch to @trigger_error('....', E_USER_DEPRECATED) when the constructor is called without an array, so that we can add the actual type hint in Drupal 11.

Also I haven't sent it for a retest because DrupalCI is having trouble, but the last green run on the patch was from 9.3.x, it's good to check things are green on both 9.5.x and 10.1.x (if they're eligible for both branches) before RTBCing.

You mean something like this?

+++ b/core/modules/editor/src/Ajax/EditorDialogSave.php
@@ -15,17 +15,20 @@ class EditorDialogSave implements CommandInterface {
   public function __construct($values) {
+    if (!is_array($values)) {
+      @trigger_error('Constructor $values must be of type array. It was added in drupal:10.1.0 and will be required before drupal:11.0.0.', E_USER_DEPRECATED);
+    }
     $this->values = $values;

So yes, but this should something like 'The $values parameter to ' . __METHOD___ . ' must be an array' so people know where the error has come from. Should be existing constructor deprecation examples in 9.5.x using __METHOD__ to copy (usually for missing arguments but this is similar enough).

@catch,

I made the changes you required at comments #25

Needs review.

@Ratan Priya can you please upload an interdiff if you're going to upload a patch please.

@smustgrave,

Added interdiff against #27

Thankyou.

Thank you!

Looking at #27 appears it's just missing the E_USER_DEPRECATED variable.

Example copied from core

@trigger_error('Passing a \'throw_exception\' option to ' . __METHOD__ . ' is deprecated in drupal:9.2.0 and is removed in drupal:10.0.0. Always catch exceptions. See https://www.drupal.org/node/3201187', E_USER_DEPRECATED);

@catch do you want this ticket number in the comment?

@smustgrave,

Made changes as per the example given in #30

needs review.

Looks good. Will let the test runner complete before marking RTBC.

Deprecation added per #25 looks good.

- Retested the patch #31 and now it seems to pass.
- The issue summary is clear and describes the problem
- Code is simple and does the type change along with triggering a depreciation error as expected

Marking this issue as RTBC

+++ b/core/modules/editor/src/Ajax/EditorDialogSave.php
@@ -15,17 +15,20 @@ class EditorDialogSave implements CommandInterface {
+    if (!is_array($values)) {
+      @trigger_error('The $values parameter to ' . __METHOD___ . ' must be an array', E_USER_DEPRECATED);
+    }

This deprecation message should say when we're going to fix this and add a typehint. I think it should be something like:

@trigger_error('Calling \Drupal\editor\Ajax\EditorDialogSave::__construct with a non-array $values parameter is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.', E_USER_DEPRECATED);

FWIW I'm not convince that this is ever called with a non-array anywhere by anything. Here's the code and docs that is triggered by this:

  /**
   * Command to save the contents of an editor-provided modal.
   *
   * This command does not close the open modal. It should be followed by a
   * call to `Drupal.AjaxCommands.prototype.closeDialog`. Editors that are
   * integrated with dialogs must independently listen for an
   * `editor:dialogsave` event to save the changes into the contents of their
   * interface.
   *
   * @param {Drupal.Ajax} [ajax]
   *   The Drupal.Ajax object.
   * @param {object} response
   *   The server response from the ajax request.
   * @param {Array} response.values
   *   The values that were saved.
   * @param {number} [status]
   *   The status code from the ajax request.
   *
   * @fires event:editor:dialogsave
   */
  Drupal.AjaxCommands.prototype.editorDialogSave = function (
    ajax,
    response,
    status,
  ) {
    $(window).trigger('editor:dialogsave', [response.values]);
  };

So we document that response.values is expected to be an array. There are other places in the JS code base where we expect an array here so I'm not 100% that this check is necessary. And maybe we could just add the typehint...

Updating patch as suggested in comment #36

@alexpott see what you are saying but would this just add an extra layer IF a string is passed. If never triggered maybe we can remove later down the line

Added patch against #37 in Drupal 10.1.x

@Shubham Sharma: Rerolls are not automatically needed if a Drupal dev branch version changes. In many cases, the patch still applies. If you click the "Add test / retest" link on the most recent patch of a given issue, you can test the patch on 10.1.x (or whatever the current dev branch is). If the patch applies there, then no reroll is needed.

You added a reroll that was not needed - I noticed you doing the same in at least 7 other recent issues. Credit is not granted for unnecessary rerolls.

Reviewing #37 it appears to be adding the typehint already (array $values) {

Agreed, typehint is there, and what I can see in contrib modules like https://git.drupalcode.org/project/ckeditor5_embedded_content/-/blob/1.0... they are also passing arrays. Testing this patch and the contrib module also continues to work fine + PhpStorm stops complaining about the value as an array not matching the string hint. I believe this can be RTBC.

1. +++ b/core/modules/editor/src/Ajax/EditorDialogSave.php
   @@ -15,17 +15,20 @@ class EditorDialogSave implements CommandInterface {
      protected $values;
   

   Let's add a parameter typehint while we're at it.

2. +++ b/core/modules/editor/src/Ajax/EditorDialogSave.php
   @@ -15,17 +15,20 @@ class EditorDialogSave implements CommandInterface {
   -  public function __construct($values) {
   +  public function __construct(array $values) {
   +    if (!is_array($values)) {
   +      @trigger_error('Calling \Drupal\editor\Ajax\EditorDialogSave::__construct with a non-array $values parameter is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.', E_USER_DEPRECATED);
   +    }
   

   If we have the typehint then the deprecation is pointless because it will never occur.

   I think in this instance adding the typehint is okay given the earlier comment.

Updated as per feedback in #45. Hiding patches in favour of merge request.

Feedback appears to have been addressed.

Updated the issue summary slightly to use standard template.

Can't quite believe it took us four years to commit pretty much the original patch from #5, but here we are.

Not backported just in case this breaks some strange edge case somewhere, although you are probably doing something wrong if you run into this.

Committed d2d6d0c and pushed to 11.x. Thanks!