Convert custom path alias forms to entity forms

1. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
   @@ -50,13 +53,26 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setRequired(TRUE)
   +      ->addPropertyConstraints('value', [
   +        'Regex' => [
   +          'pattern' => '/^\//i',
   +          'message' => new TranslatableMarkup('The source path has to start with a slash.'),
   +        ],
   +      ])
   +      ->addPropertyConstraints('value', ['ValidPath' => []]);
   

   oh wow, I don't think I've seen that before (the regex).

   But wondering if we could just do that as part of ValidPath?

2. +++ b/core/lib/Drupal/Core/Path/Plugin/Validation/Constraint/UniquePathAliasConstraintValidator.php
   @@ -0,0 +1,82 @@
   +/**
   + * Constraint validator for a unique path alias.
   + */
   +class UniquePathAliasConstraintValidator extends ConstraintValidator implements ContainerInjectionInterface {
   +
   +  /**
   +   * The entity type manager.
   +   *
   

   we can't re-use UniqueFieldValueValidator for this I guess?

3. +++ b/core/modules/path/config/optional/language.content_settings.path_alias.path_alias.yml
   @@ -0,0 +1,8 @@
   +dependencies: {  }
   +id: path_alias.path_alias
   +target_entity_type_id: path_alias
   +target_bundle: path_alias
   +default_langcode: und
   +language_alterable: true
   

   I know from redirect.module that the default language widget options are very confusing for path/redirect and I think we ended up customizing that.

4. +++ b/core/modules/path/path.module
   @@ -23,20 +28,66 @@ function path_help($route_name, RouteMatchInterface $route_match) {
   +      'settings' => [
   +        'include_locked' => FALSE,
   +      ],
   
   @@ -69,3 +120,33 @@ function path_entity_translation_create(ContentEntityInterface $translation) {
   +    if ($field_name === 'langcode') {
   +      $element['value']['#description'] = t('A path alias set for a specific language will always be used when displaying this page in that language, and takes precedence over path aliases set as <em>- Not specified -</em>.');
   +      $element['value']['#empty_value'] = LanguageInterface::LANGCODE_NOT_SPECIFIED;
   +      $element['value']['#empty_option'] = t('- Not specified -');
   +    }
   +
   

   Ah, I see, you did that too :)

5. +++ b/core/modules/path/path.module
   @@ -69,3 +120,33 @@ function path_entity_translation_create(ContentEntityInterface $translation) {
   +    if ($field_name === 'path') {
   +      $element['value']['#description'] = t('Specify the existing path you wish to alias. For example: /node/28, /forum/1, /taxonomy/term/1.');
   +    }
   +
   +    if ($field_name === 'alias') {
   +      $element['value']['#description'] = t('Specify an alternative path by which this data can be accessed. For example, type "/about" when writing an about page.');
   

   wondering if it wouldn't be cleaner to put this stuff in PathAliasForm, that's how we do it on other entity types like node too?

   And we do use the field description by default, but often these descriptions don't make too much sense as the general field description and vice-versa.

6. +++ b/core/modules/path/src/Controller/PathController.php
   @@ -99,11 +99,11 @@ public function adminOverview(Request $request) {
            'title' => $this->t('Edit'),
   -        'url' => Url::fromRoute('path.admin_edit', ['pid' => $data->pid], ['query' => $destination]),
   +        'url' => Url::fromRoute('entity.path_alias.edit_form', ['path_alias' => $data->pid], ['query' => $destination]),
          ];
          $operations['delete'] = [
            'title' => $this->t('Delete'),
   -        'url' => Url::fromRoute('path.delete', ['pid' => $data->pid], ['query' => $destination]),
   +        'url' => Url::fromRoute('entity.path_alias.delete_form', ['path_alias' => $data->pid], ['query' => $destination]),
          ];
          $row['data']['operations'] = [
   

   This feels icky, thought we could use list builder operations already or something, but we're refactoring all of this anyway in the list builder issue.

7. +++ b/core/modules/path/src/Form/PathFilterForm.php
   @@ -65,7 +65,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
       */
      public function resetForm(array &$form, FormStateInterface $form_state) {
   -    $form_state->setRedirect('path.admin_overview');
   +    $form_state->setRedirect('entity.path_alias.collection');
      }
    
   

   makes me wonder if pathauto or redirect use some of these old routes anywhere..

#2336597: Convert path aliases to full featured entities is in.

Watching this since it may be helpful or a blocker for #3085480: \Relax PathAliasConstraint when creating draft content and instead create pending alias revisions.

+++ b/core/lib/Drupal/Core/Path/Plugin/Validation/Constraint/UniquePathAliasConstraintValidator.php
@@ -0,0 +1,82 @@
+  private $entityTypeManager;
...
+    $this->entityTypeManager = $entity_type_manager;
...
+    $storage = $this->entityTypeManager->getStorage('path_alias');

Constructor can get storage instead of ETM, also why the property is private?

+++ b/core/lib/Drupal/Core/Path/Plugin/Validation/Constraint/UniquePathAliasConstraintValidator.php
@@ -14,20 +14,20 @@
 class UniquePathAliasConstraintValidator extends ConstraintValidator implements ContainerInjectionInterface {
...
-   * The entity type manager.
+   * The path alias entity storage.
    *
-   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
+   * @var \Drupal\Core\Entity\ContentEntityStorageInterface
    */
-  private $entityTypeManager;
+  protected $pathAliasStorage;

private was definitely wrong, not quite so sure about the entity type manager -> storage change. IMHO injecting non-services can be a bit problematic. We already had to add special support for entity storages to \Drupal\Core\DependencyInjection\DependencySerializationTrait for example.

@berdir as #17 said ...is there existing issue about it? There's both ways used in core now
Personally I prefer to require in constructor only storage cos class has "less knowledge" about external dependencies

With deprecation tests and everything, beautiful ;)

@andypost: Yeah, I'm happy to argue why I think it's better the inject the entity type manager only in a separate issue (in short, performance when not used and ability to clear storages when certain changes happen. Might not be too relevant for this validator, but matters in other cases).

I think the main point from my review that wasn't addressed is altering the form elements in the widget alter or in the entity form. I prefer doing something like that in the EntityForm class instead of putting things in the .module file, but that's more a personal preference and I don't feel strongly about it. I'll leave that decision to the core maintainer who is going to review this. @amateescu correctly said that we don't do that yet in core in e.g. NodeForm, but we don't have any comparable widget alter hooks either I think, commerce seems to do that a lot though ;) And most core content entity forms unfortunately don't use widgets at all.

I did some manual tests as well and as far as I see, the add/edit forms look identical. There are tiny differences on the delete form (Confirm button becomes Delete, and the confirm message has an extra "the", but that's standardized on entity delete forms and actually seems more correct).

1. We need a change record I think...
   

   +++ b/core/modules/path/src/Routing/RouteProcessor.php
   @@ -0,0 +1,69 @@
   +    if (in_array($route_name, array_keys($redirected_route_names), TRUE)) {
   +      @trigger_error("The '{$route_name}' route is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use the '{$redirected_route_names[$route_name]}' route instead.", E_USER_DEPRECATED);
   +      static::overwriteRoute($route, $this->routeProvider->getRouteByName($redirected_route_names[$route_name]));
   +    }
   

   And the deprecation messages need to link to said change record. Ah I see we are going to update the existing CR - well that CR needs to link to this issue and so do the deprecation messages.

2. +++ b/core/modules/locale/tests/src/Functional/LocalePathTest.php
   @@ -69,18 +69,18 @@ public function testPathLanguageConfiguration() {
        $edit = [
   -      'source'   => '/node/' . $node->id(),
   -      'alias'    => '/' . $english_path,
   -      'langcode' => 'en',
   +      'path[0][value]' => '/node/' . $node->id(),
   +      'alias[0][value]' => '/' . $english_path,
   +      'langcode[0][value]' => 'en',
        ];
   

   This change is likely to be disruptive but form arrays are not API so this is allowed to change in a minor. Hopefully won't be too painful for contrib and custom tests.

3. +++ /dev/null
   @@ -1,33 +0,0 @@
   -<?php
   -
   -namespace Drupal\path\Form;
   -
   

   The good thing is that these form class removals would appear not to break contrib - http://grep.xnddx.ru/search?text=Drupal%5Cpath%5CForm&filename=

4. +++ b/core/modules/path/src/Routing/RouteProcessor.php
   @@ -0,0 +1,69 @@
   +   * Constructs a RestResourceGetRouteProcessorBC object.
   

   This is not a RestResourceGetRouteProcessorBC

5. +++ b/core/tests/Drupal/FunctionalTests/Hal/PathAliasHalJsonTestBase.php
   @@ -43,14 +43,4 @@ protected function getNormalizedPostEntity() {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  protected function getExpectedCacheContexts() {
   -    return [
   -      'url.site',
   -      'user.permissions',
   -    ];
   -  }
   
   +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasResourceTestBase.php
   @@ -109,11 +114,4 @@ protected function getNormalizedPostEntity() {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  protected function getExpectedCacheContexts() {
   -    return ['user.permissions'];
   -  }
   

   First reaction is that this seems unrelated - how come these changes are necessary?

So this change is going to break some contrib - see
http://grep.xnddx.ru/search?text=_form_path_&filename=

So https://www.drupal.org/project/fixed_path_alias https://www.drupal.org/project/mpac and https://www.drupal.org/project/views_url_alias

Discussed with @catch and @plach and consider whether we should put this in as a breaking change in 9.0.x but they both felt it was better to go in 8.8.x and I agreed.

Committed f9528c5 and pushed to 9.0.x and cherry-picked to 8.9.x and 8.8.x. Thanks!

Found this gold #147143: Add new URL aliases UI and moved it to 9.1.x

Updating the release note with disruption information from @pameeela, and crediting her for the improvement

And actually pasting the draft snippet.