Catch and handle exceptions in PathFieldItemList

New patch on top of #2336597: Convert path aliases to full featured entities

Reroll as per #2336597-173: Convert path aliases to full featured entities

Reroll after #3084983: Move all the code related to path aliases to a new (required) "path_alias" module.

Thanks updated the patch.

Thanks, for adding the tests. Patch looks good just some minor observations.

1. +++ b/core/modules/path/tests/modules/path_entity_test/path_entity_test.module
   @@ -0,0 +1,35 @@
   +      ->setDisplayConfigurable('form', TRUE)
   

   We don't need this.

2. +++ b/core/modules/path/tests/modules/path_entity_test/src/Entity/PathEntityTestExternalLink.php
   @@ -0,0 +1,33 @@
   +  public function toUrl($rel = 'canonical', array $options = []) {
   +    return Url::fromUri('http://example.com/');
   +  }
   

   FYI: this is what linky module is doing as well.

3. +++ b/core/modules/path/tests/src/Kernel/PathFieldItemListTest.php
   @@ -0,0 +1,75 @@
   +    $this->assertEquals('', $entity->path->value);
   ...
   +    $this->assertNull($entity->path->value);
   ...
   +    $this->assertNull($entity->path->value);
   

   Why the first one is also not asserNull?

4. +++ b/core/modules/path/tests/src/Kernel/PathFieldItemListTest.php
   @@ -0,0 +1,75 @@
   +    // Entities with canonical URLS to external (aka unrouted) throw
   

   s/URLS/URLs

I have checked the interdiff and patch manually looks good to me. Implemented as per #14.
Let's wait for the patch to pass the automated tests

Thanks, for addressing the feedback.

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -26,15 +28,26 @@ class PathFieldItemList extends FieldItemList {
+      catch (EntityMalformedException $exception) {
+      }
+      catch (UndefinedLinkTemplateException $exception) {
+      }
+      catch (\UnexpectedValueException $exception) {
+      }

Let's add some comments here to explain why and in which case these exceptions will be thrown.

That makes sense. I was just rerolling the patch for the most part so I think it is fair to RTBC the patch.

Another reroll.

One thing that's missing from the issue summary is why entity types that have the special path computed field. In core path_entity_base_field_info() adds the field to 'taxonomy_term', 'node', 'media' entity types but none of them have the problems outlined in the issue summary. So why are entity types having this path field added?

So I guess the question then is why would you enable pathauto on an entity type that would cause this. Which entity types get the path field is user configurable.

  // Verify that the configuration data isn't null (as is the case before the
  // module's initialization, in tests), so that in_array() won't fail.
  if ($enabled_entity_types = $config->get('enabled_entity_types')) {

It feels like that is a configuration error though and something pathauto should try and detect and tell the user about.

I worry that this change will hide helpful error messages when a developer is creating custom code.

> If entity doesnt implement 'canonical' link template, \Drupal\Core\Entity\Exception\UndefinedLinkTemplateException is thrown.
> If canonical URL is not routable, usually when URL is external.

Couldn't these problems be picked up on entity type rebuild, and exceptions thrown at that point? That would actually improve DX: better to have the exception thrown when you enable the module or do drush cr than when you view an entity.

> I worry that this change will hide helpful error messages when a developer is creating custom code.

I agree.

> If entity doesnt implement 'canonical' link template, \Drupal\Core\Entity\Exception\UndefinedLinkTemplateException is thrown.

Don't let the field be added to this entity type.

An exception should be thrown at SOME point to alert a developer to the problem.

> If canonical URL is not routable, usually when URL is external.

Possibly the same -- don't allow the field to be added to this entity type?

> If entity is unsaved, \Drupal\Core\Entity\EntityMalformedException is thrown. (may not be an issue since path list already checks isNew).

Possibly not an issue! Sounds like this case should be investigated more.

Settings to needs work.