Add fieldset/vertical tab for URL alias field

Nice improvement to the interface!

+++ b/core/modules/path/path.module
@@ -40,23 +41,28 @@ function path_help($route_name, RouteMatchInterface $route_match) {
 /**
  * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\node\NodeForm.
  */
-function path_form_node_form_alter(&$form, FormStateInterface $form_state) {
...
+function path_form_alter(&$form, FormStateInterface $form_state) {

I suppose this function doc should be updated to new hook implementation.

This patch looks good! I really like this change. I have some nits to pick though.

1. +++ b/core/modules/path/tests/src/Functional/PathMediaFormTest.php
   @@ -0,0 +1,68 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Should be {@inheritdoc}

2. +++ b/core/modules/path/tests/src/Functional/PathMediaFormTest.php
   @@ -0,0 +1,68 @@
   +    $media_type_id = strtolower($this->randomMachineName());
   

   randomMachineName should not be used here, (see https://www.drupal.org/project/drupal/issues/2571183#comment-12350702). I think this will be just as good with any string as id.

Looks solid! I don't think I can RTBC this before this gets that usability review though.

This also means we're hiding things behind yet another click. On the content form this is the case as well, but we may assume pathauto being used there more often. But l'll take the consistency argument. This is mostly a one-time set it and forget it task so I think it's fine to proceed here.

Setting to RTBC based on @yoroy's review on the usuability side and my review for the code.

+++ b/core/modules/path/path.module
@@ -38,25 +39,30 @@ function path_help($route_name, RouteMatchInterface $route_match) {
+function path_form_alter(&$form, FormStateInterface $form_state) {
+  $form_object = $form_state->getFormObject();
+  if ($form_object instanceof ContentEntityFormInterface) {
+    $entity = $form_state->getFormObject()->getEntity();
+    if ($entity->hasField('path')) {
+      $form['path_settings'] = [

\Drupal\path\Plugin\Field\FieldWidget\PathWidget::formElement() does have access to the full form?

Can we maybe move the logic in there, so we don't need a global form alter hook that is called on every form?

CommentWidget::formElement() is for example already doing that, based on the existence of a $form['advanced'] element.

A much nicer solution! Needs another review though...

+++ b/core/modules/path/tests/src/Functional/PathMediaFormTest.php
@@ -0,0 +1,66 @@
+
+    // Make sure we have a Path fieldset and Path fields.
+    $assert_session->responseContains(' id="edit-path-0"');
+    $assert_session->fieldExists('path[0][alias]');
...
+
+    // See if the whole fieldset is gone now.
+    $assert_session->responseNotContains(' id="edit-path-0"');
+    $assert_session->fieldNotExists('path[0][alias]');

Yeah, not sure about the raw ID check here either. Why don't we just look for vertical tab title (URL Alias settings), at least additionally to the ID?

Looking into this

When I applied the above patch to 8.6.x branch I did not get the expected results from the screenshots above. On both pages I saw the field displayed normally like the before patch screenshots.

Steps taken

• git clone --branch 8.6.x https://git.drupal.org/project/drupal.git
• Apply patch
• clear cache
• review taxonomy add term
• review media form

+++ b/core/modules/path/path.module
@@ -6,6 +6,7 @@
+use Drupal\Core\Entity\ContentEntityFormInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Field\BaseFieldDefinition;
 use Drupal\Core\Form\FormStateInterface;

Neither ContentEntityFormInterface or FormStateInterface is used in path.module, this can be deleted.

@berdir regarding #14, can you be more specific? I am still getting used to writing tests so an example of where we are already doing something similar would be hugely helpful.

The new test is looking for $form['advanced']. I'm not sure why it wouldn't be visible at least for media, that should get this from the parent in \Drupal\Core\Entity\ContentEntityForm::form? . I don't think it's the responsibility of this issue to define a vertical tabs element if it doesn't exist yet. Previous it always defined the details element but I guess the #group => 'advanced' just did nothing if it wasn't defined?

Not sure if the goal of this issue is to only add it to a vertical tabs/sidebar element if that exists or to always define a wrapper? This is now consistent with \Drupal\comment\Plugin\Field\FieldWidget\CommentWidget, but that was also specifically done like that to continue working as it did before for nodes.

We also have issues to generalize the node form sidebar thing in seven to other entity forms.

For the test, something like this is what I meant:

$assert_session->elementContains('css', '#edit-path-0', 'URL path settings');

Alternatively, you can also use the find* methods on getSession()->getPage() and then from that, use getText() or getHTML() and work with $this->assertContains() or so:

$page->find('css', '#edit-path-0')->getText()

Alright so I spun up two simplytest envs, one at patch #2 and one at patch #12 (password admin/admin).

#2 works for both media and taxonomy while #12 does not. Reviewing the interdiff for #12 shows that the hook_form_alter was removed but the following steps outlined in #11 were not completed.

Finally, I updated the test per #18.

Tried to replicate locally but was getting a different error

Behat\Mink\Exception\ElementNotFoundException : Element matching css "#edit-path-0" not found.

while the automatic test failure was

Behat\Mink\Exception\ElementHtmlException: The string "URL path settings" was not found in the HTML of the element matching css "#edit-path-0".

Wanted to get this quick change up to see if it passes.

+++ b/core/modules/path/path.module
@@ -37,28 +38,6 @@ function path_help($route_name, RouteMatchInterface $route_match) {
-/**
- * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\node\NodeForm.
- */
-function path_form_node_form_alter(&$form, FormStateInterface $form_state) {
-  $node = $form_state->getFormObject()->getEntity();
-  $form['path_settings'] = [
-    '#type' => 'details',
-    '#title' => t('URL path settings'),
-    '#open' => !empty($form['path']['widget'][0]['alias']['#value']),
-    '#group' => 'advanced',
-    '#access' => !empty($form['path']['#access']) && $node->hasField('path') && $node->get('path')->access('edit'),
-    '#attributes' => [
-      'class' => ['path-form'],
-    ],
-    '#attached' => [
-      'library' => ['path/drupal.path'],
-    ],
-    '#weight' => 30,
-  ];
-  $form['path']['#group'] = 'path_settings';
-}
-

Question -- won't this end up removing the URL alias field if the 'advanced' tab set is unavailable? If so, this patch could theoretically introduce a regression. We might want to expand the test coverage to ensure that it's visible in some place where it previously appeared despite there being no "advanced" tab (if such a place exists in core).

Reviewed the patch and it looks good to me

This is really great, and cleans up the form so much!! Especially with remote video because then you don't have that utterly perplexing "Remote URL" and "URL alias" combo pak.

The one concern I had is that the test for making sure we don't vertical-tab this field on non-revisionable entities is targeting taxonomy terms, and while that should be fine for 8.6, in 8.7 we definitely hope to have those (as well as menu items + comments) revisionable, for an improved content staging experience. #2880149: Convert taxonomy terms to be revisionable has activity on it as of 11 hours ago, so is actively being worked on by that team, whereas this is "just" a UX improvement.

So could we instead either a) target something that's not one of the "main" content entities (shortcuts, maybe?) or b) invent our own "mock" non-revisionable entity for testing purposes?

Otherwise, this is a lovely improvement filled with loveliness, and I would love to see it in 8.6!

Oh one other tiniest nitpick we found during review:

+  /**
+   * Tests the node form ui.
+   */
+  public function testMediaForm() {

Should be "media form ui" (and why not "UI" while we're at it, lol).

MOAR credits!

It turns out that we cannot test with shortcuts because Path has support for node, media, and taxonomy hard-coded. Wow.

From path_entity_base_field_info():

if (in_array($entity_type->id(), ['taxonomy_term', 'node', 'media'], TRUE)) {
    $fields['path'] = BaseFieldDefinition::create('path')

Yeah.

So in discussion with @webchick, we agreed to remove the new taxonomy term form test from the patch, since #2880149: Convert taxonomy terms to be revisionable will render it entirely useless anyway.

Looks good to me! Setting to RTBC.

Awesome! Committed and pushed to 8.7.x and backported to 8.6.x. Thanks!

#2989950: Fix Unused use in path.module follow up for added phpcs failure.