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.)
Comments
Comment #1
miro_dietikerWhen 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.
Comment #2
dawehnerWell, 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?
Comment #3
ArlaYes. With "gracefully" I mean avoiding an uncaught exception.
Comment #4
ArlaHere's throwing an exception in EntityType::__construct.
Comment #6
miro_dietikerThis 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.
Comment #7
dawehnerWhat about throwing this exception in
\Drupal\Core\EntityManager::processDefinition()
in which case, this would be just checked once and not during runtime?Comment #8
ArlaSounds perfect to me.
Comment #10
ArlaOops, 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.Comment #11
dawehnerYou can also directly cast it to an array.
Comment #12
ArlaYes, that is perhaps nicer.
Also added unit test case and included @entity_type in exception message.
Comment #13
dawehnerPerfect, thank you!
Comment #14
alexpottWhy don't we just add a leading slash on if it is missing?
Comment #15
BerdirDoing 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 /" ?
Comment #16
BerdirSetting to needs work for my suggested message above.
Comment #17
miro_dietikerAlso the patch doesn't apply anymore.
Comment #18
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedRerolled and changed the warning message.
Comment #19
BerdirBack 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.
Comment #20
dawehnerWe no longer use safemarkup for exceptions, better to use
sprintf()
instead.It is great to have a unit test.
Comment #21
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedChanged
SafeMarkup
tosprintf()
.Comment #22
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedComment #24
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedSorry about that, fixed the syntax now.
Comment #25
dawehnerCool thank you!
Comment #26
alexpottLink 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'
Comment #27
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedChanged the exception message as suggested.
Comment #28
alexpottThis 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.
Comment #29
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedRemoved the
sprintf
function and the full stop.Comment #30
dawehnerSorry yeah I totally forgot we even don't do sprintf anymore
Comment #31
Wim LeersOh! Since when?
Updated the title, because that made me think we automatically fix it.
Comment #32
alexpottThis 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!
Comment #34
miro_dietikerVery 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!