Problem/Motivation

Enabling config_translation throws an uncaught exception if there are modules installed with link templates without leading slash:

exception 'InvalidArgumentException' with message 'Link templates accepts paths, which have to start with a leading slash.' in [error]
/usr/local/var/www/d8/www/core/lib/Drupal/Core/Entity/EntityType.php:582
Stack trace:
#0 /usr/local/var/www/d8/www/core/modules/config_translation/config_translation.module(95): Drupal\Core\Entity\EntityType->setLinkTemplate('config-translat...', 'admin/structure...')
#1 /usr/local/var/www/d8/www/core/lib/Drupal/Core/Extension/ModuleHandler.php(494): config_translation_entity_type_alter(Array, NULL, NULL)
#2 /usr/local/var/www/d8/www/core/lib/Drupal/Core/Entity/EntityManager.php(245): Drupal\Core\Extension\ModuleHandler->alter('entity_type', Array)
...

Proposed resolution

Catch the exception, or validate link templates before passing them to setLinkTemplate() (perhaps in EntityType::hasLinkTemplate()).

Or maybe validate and warn already when reading the entity type definition? (If I were an entity type, I would appreciate it if people told me immediately when my link templates are invalid.)

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker’s picture

When actively developing with TMGMT, i ran into this issue with multiple contrib modules and needed to fix them.

This issue is important as it contains a severe fatal error risk.
If a developer usually only works with a single language and everything seems to work in a single language environment, installing the same module in a multilingual environment immediately breaks everything.

dawehner’s picture

Well, I don't get all that fail gracefully ... don't we want to fail early instead, so also fail in case you don't have the multilingual toolsuite available, so developers are never able to make the mistake?

Arla’s picture

Yes. With "gracefully" I mean avoiding an uncaught exception.

Arla’s picture

Status: Active » Needs review
FileSize
1.69 KB

Here's throwing an exception in EntityType::__construct.

Status: Needs review » Needs work

The last submitted patch, 4: link-templates-2496897-4.patch, failed testing.

miro_dietiker’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -10,6 +10,7 @@
@@ -253,6 +254,16 @@ public function __construct($definition) {
+    foreach ($definition['links'] as $link_relation_name => $link_template) {

This adds overhead to every entity type construct. That is a really bad idea.

The problem is triggered with/by config translation that just assumes that the link template is well formatted. I think the error/exception originated there should be caught.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -253,6 +254,16 @@ public function __construct($definition) {
 
+    // All link templates must have a leading slash.
+    foreach ($definition['links'] as $link_relation_name => $link_template) {
+      if ($link_template[0] != '/') {
+        throw new InvalidLinkTemplateException(SafeMarkup::format('Link template for @relation is missing leading slash: @template.', array(
+          '@relation' => $link_relation_name,
+          '@template' => $link_template,
+        )));
+      }
+    }
+

What about throwing this exception in \Drupal\Core\EntityManager::processDefinition() in which case, this would be just checked once and not during runtime?

Arla’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
2.53 KB

Sounds perfect to me.

Status: Needs review » Needs work

The last submitted patch, 8: link-templates-2496897-8.patch, failed testing.

Arla’s picture

Status: Needs work » Needs review
FileSize
988 bytes
2.1 KB

Oops, sorry for not running any tests.

Missed that $definition is typed (not array) in processDefinition(). Also added ?: array() because getLinkTemplates() returns NULL when mocked in unit tests.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -231,10 +231,11 @@ public function clearCachedDefinitions() {
+    foreach ($definition->getLinkTemplates() ?: array() as $link_relation_name => $link_template) {

You can also directly cast it to an array.

Arla’s picture

Yes, that is perhaps nicer.

Also added unit test case and included @entity_type in exception message.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Why don't we just add a leading slash on if it is missing?

Berdir’s picture

Doing it in EntityType::__construct() is fine actually. That's created from the annotation and then cached as an object, __construct() isn't ever called again until the next cache clear.

I'd say educating developers that the leading / is required is fine. We actually don't know if the missing leading / is the only problem.

Most time that we've seen this problem is after the route name => link pattern conversion. Unlikely to happen again but who knows, people might add bogus content in there and adding a / would just add further confusion, so +1 to an exception. Wondering if the exception message should be a bit more generic and say something like "... must be a well formatted link template including leading /" ?

Berdir’s picture

Status: Needs review » Needs work

Setting to needs work for my suggested message above.

miro_dietiker’s picture

Also the patch doesn't apply anymore.

sasanikolic’s picture

Rerolled and changed the warning message.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, see explanation in #15 why we validate and don't auto-fix.

There are still contrib modules that are doing it incorrectly, see #2521694: Fix entity link templates and corresponding route names for example. So it would be nice to have that validated.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -229,6 +230,25 @@ public function clearCachedDefinitions() {
    +        throw new InvalidLinkTemplateException(SafeMarkup::format('Link template @relation for @entity_type must be a well formatted link template including leading /: @template.', array(
    

    We no longer use safemarkup for exceptions, better to use sprintf() instead.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -266,6 +266,25 @@ public function testClearCachedDefinitions() {
    +   * @expectedException \Drupal\Core\Entity\Exception\InvalidLinkTemplateException
    +   * @expectedExceptionMessage Link template canonical for apple must be a well formatted link template including leading /: path/to/apple
    +   */
    

    It is great to have a unit test.

sasanikolic’s picture

sasanikolic’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: fail_gracefully_if_link-2496897-21.patch, failed testing.

sasanikolic’s picture

Sorry about that, fixed the syntax now.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Link template canonical for apple must be a well formatted link template including leading /: path/to/apple
I think the end of this exception message is a bit confusing... I'm left wondering what emoticon /: is.

How about:

Link template 'canonical' for entity type 'apple' must start with a leading slash, the current link template is 'path/to/apple'

sasanikolic’s picture

Changed the exception message as suggested.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -237,7 +237,7 @@ public function processDefinition(&$definition, $plugin_id) {
+        throw new InvalidLinkTemplateException(sprintf("Link template " . "'" . "%s" . "'" . " for entity type " . "'" . "%s" . "'" . " must start with a leading slash, the current link template is " . "'" . "%s" . "'.", $link_relation_name, $plugin_id,  $link_template));

This can just be:
throw new InvalidLinkTemplateException("Link template '$link_relation_name' for entity type '$plugin_id' must start with a leading slash, the current link template is '$link_template'");

Note not full stop on the end.

sasanikolic’s picture

Removed the sprintf function and the full stop.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Sorry yeah I totally forgot we even don't do sprintf anymore

Wim Leers’s picture

Title: Fail gracefully if link templates are missing leading / » Throw helpful exception if link templates are missing leading /

Sorry yeah I totally forgot we even don't do sprintf anymore

Oh! Since when?


Updated the title, because that made me think we automatically fix it.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed c2f5a3e and pushed to 8.0.x. Thanks!

  • alexpott committed c2f5a3e on 8.0.x
    Issue #2496897 by sasanikolic, Arla, dawehner, miro_dietiker, Berdir:...
miro_dietiker’s picture

Very nice! This immediately led to test fails with some of our contrib (eg. monitoring #2535370: SensorConfig link template is missing a leading slash) and leads to much better module quality.

Thx for pushing!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.