Create a new OpenModalDialogWithUrl command

Thanks for the feature suggestion @srishtiiee.

Reviewed the Merge Request. The changes look good and working as expected.
Tested the same on a Drupal 11 installation.
Testing steps:

• Setup a Drupal 11 site and applied the patch from the MR
• Created a Custom Ajax form with a button and a custom submit handler.
• Inside the handler added the code snippet to open a node in a dialog.
  

  $response = new AjaxResponse();
    $url = Url::fromRoute('entity.node.canonical', ['node' => 1]);
    $command = new OpenModalDialogWithUrl($url->toString(),
    [
      'url' => 'example',
      'width' => 500,
      'title' => 'Title',
      'modal' => TRUE,
    ]);
    $response->addCommand($command);
    return $response;

• The node gets displayed in a dialog as expected.

Attaching the screenshots.

This looks RTBC for me.

Feels like something that will need a CR

I am adding #1886616: Multistep Form Wizard as a related issue. I have not looked at it closely, but I think that is more specialized than this issue, trying to make it easier to create multi-step entity forms.

Updated the already existing CR for this.

1) Drupal\Tests\Core\Ajax\AjaxCommandsTest::testOpenModalDialogWithUrl
Error: Class "Drupal\Core\Ajax\OpenModalDialogWithUrl" not found
/builds/issue/drupal-3408738/core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php:509
/builds/issue/drupal-3408738/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3408738/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3408738/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3408738/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3408738/vendor/phpunit/phpunit/src/TextUI/Command.php:97
ERRORS!
Tests: 32, Assertions: 37, Errors: 1.

CR reads well.

Applied the MR and used the snippet in the CR to open a dialog. I used a node add form, as that's something I'm currently working on in my project.

Reviewing the code and believe this is good

+1 to RTBC, the Drupal.ajax vs ajax discussion in the MR can be resolved

There are a bunch of small inconsistencies in the code. Some parameters/decisions should be documented. And most importantly: neither this MR nor the CR document how this relates to the multiple existing dialog-related AJAX commands. "When to use which?" is a critical portion of landing this.

And most importantly: neither this MR nor the CR document how this relates to the multiple existing dialog-related AJAX commands. "When to use which?" is a critical portion of landing this.

Updated the CR to address this.

CR appears to have been updated with a usage.

Not sure where documentation should go though. Assuming here https://www.drupal.org/docs/drupal-apis/ajax-api/core-ajax-callback-comm... but imagine the update should be after the code is committed right?

This is still missing crucial documentation, so it can't be RTBC.

Plus, based on the test coverage I just realized that this appears to be introducing a new potential attack surface 😰 So tagging Needs security review as well…

Set to needs work for writing a better test.

Detailed my security concerns at https://git.drupalcode.org/project/drupal/-/merge_requests/5810#note_272697.

The hardening and test coverage looks good to me.

Have posted to #security-discussion in slack so fingers crossed someone can pick up.

The aspect that required security review has been addressed. I think a regular committer review is sufficient now.

Yes, agreed, the security concerns I raised are now addressed 👍

Just a few remarks to ensure A) docs are discoverable by developers, B) this stays aligned/in sync with the other place in core where this pattern is used.

Feedback from @Wim Leers has been addressed 👍

One more thought - we could add an extra method on UrlHelper for the local check, combining those two method calls. I was thinking about a trait, but why not just an extra helper method directly on that class.

Not for here though, so committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Follow up added #3437765: Add an extra method on UrlHelper for a local check