Problem/Motivation

Entity link templates are still route names instead of paths, and the names of the corresponding routes do not always start with entity..

Proposed resolution

  1. Convert link template values from route names to paths.
  2. Make sure the names of the routes belonging to link templates are prefixed with entity..

Remaining tasks

None.

User interface changes

None.

API changes

Route renames.

Data model changes

None.

Original report

If currency is installed and you enable devel module you get the above exception.

Additional uncaught exception thrown while handling exception.

Original

InvalidArgumentException: Link templates accepts paths, which have to start with a leading slash. in Drupal\Core\Entity\EntityType->setLinkTemplate() (line 582 of /var/www/MY-PROJECT/core/lib/Drupal/Core/Entity/EntityType.php).

Drupal\Core\Entity\EntityType->setLinkTemplate('devel-load', 'currency.currency.edit/devel')
devel_entity_type_alter(Array, NULL, NULL)
Drupal\Core\Extension\ModuleHandler->alter('entity_type', Array)
Drupal\Core\Entity\EntityManager->findDefinitions()
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('user_role', )
Drupal\Core\Entity\EntityManager->getDefinition('user_role', )
Drupal\Core\Entity\EntityManager->hasHandler('user_role', 'storage')
Drupal\user\Theme\AdminNegotiator->applies(Object)
Drupal\Core\Theme\ThemeNegotiator->determineActiveTheme(Object)
Drupal\Core\Theme\ThemeManager->initTheme(NULL)
Drupal\Core\Theme\ThemeManager->getActiveTheme()
Drupal\Core\Render\ElementInfoManager->getInfo('page')
Drupal\Core\Render\Renderer->doRender(Array, 1)
Drupal\Core\Render\Renderer->render(Array, 1)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Error', 'maintenance_page', Array)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_Render_BareHtmlPageRenderer_Proxy->renderBarePage(Array, 'Error', 'maintenance_page')
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onHtml(Object)
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onException(Object, 'kernel.exception', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object)
Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Symfony_Component_HttpKernel_HttpKernel_Proxy->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_StackMiddleware_Session_Proxy->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_StackMiddleware_KernelPreHandle_Proxy->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
Additional

InvalidArgumentException: Link templates accepts paths, which have to start with a leading slash. in Drupal\Core\Entity\EntityType->setLinkTemplate() (line 582 of /var/www/MY-PROJECT/core/lib/Drupal/Core/Entity/EntityType.php).

Drupal\Core\Entity\EntityType->setLinkTemplate('devel-load', 'currency.currency.edit/devel')
devel_entity_type_alter(Array, NULL, NULL)
Drupal\Core\Extension\ModuleHandler->alter('entity_type', Array)
Drupal\Core\Entity\EntityManager->findDefinitions()
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('currency', )
Drupal\Core\Entity\EntityManager->getDefinition('currency')
Drupal\Core\Entity\EntityManager->getHandler('currency', 'storage')
Drupal\Core\Entity\EntityManager->getStorage('currency')
Drupal\currency\Element\CurrencyAmount::create(Object, Array, 'currency_amount', Array)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('currency_amount', Array)
Drupal\Component\Plugin\PluginManagerBase->createInstance('currency_amount', Array)
Drupal\Core\Render\ElementInfoManager->createInstance('currency_amount')
Drupal\Core\Render\ElementInfoManager->buildInfo('np8_base_theme')
Drupal\Core\Render\ElementInfoManager->getInfo('page')
Drupal\Core\Render\Renderer->doRender(Array, 1)
Drupal\Core\Render\Renderer->render(Array, 1)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Error', 'maintenance_page', Array)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_Render_BareHtmlPageRenderer_Proxy->renderBarePage(Array, 'Error', 'maintenance_page')
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

This is due to the fact that in the entity annotations the leading "/" is missing.

/**
 * Defines a currency locale entity.
 *
 * @ConfigEntityType(
 *   handlers = {
 *     "access" = "Drupal\currency\Entity\CurrencyLocale\CurrencyLocaleAccessControlHandler",
 *     "form" = {
 *       "default" = "Drupal\currency\Entity\CurrencyLocale\CurrencyLocaleForm",
 *       "delete" = "Drupal\currency\Entity\CurrencyLocale\CurrencyLocaleDeleteForm"
 *     },
 *     "list_builder" = "Drupal\currency\Entity\CurrencyLocale\CurrencyLocaleListBuilder",
 *     "storage" = "Drupal\Core\Config\Entity\ConfigEntityStorage",
 *   },
 *   entity_keys = {
 *     "id" = "locale",
 *     "label" = "locale",
 *     "uuid" = "uuid",
 *     "status" = "status"
 *   },
 *   id = "currency_locale",
 *   label = @Translation("Currency currency locale"),
 *   links = {
 *     "canonical" = "currency.currency_locale.edit",
 *     "create-form" = "currency.currency_locale.add",
 *     "edit-form" = "currency.currency_locale.edit",
 *     "delete-form" = "currency.currency_locale.delete"
 *   }
 * )
 */
class CurrencyLocale extends ConfigEntityBase implements CurrencyLocaleInterface {

  use StringTranslationTrait;

...

The links pattern need a "/" in front.

I will send a patch after getting an issue number.

CommentFileSizeAuthor
#30 interdiff-fix_entity_link-2521694-27-30.txt5.68 KBedurenye
#30 fix_entity_link-2521694-30.patch13.45 KBedurenye
#27 additional_uncaught-2521694-27.patch16.85 KBedurenye
#21 interdiff-additional_uncaught-2521694-20-21.txt1.68 KBedurenye
#21 additional_uncaught-2521694-21.patch27.53 KBedurenye
#20 interdiff-additional_uncaught-2521694-17-20.txt8.02 KBedurenye
#20 additional_uncaught-2521694-20.patch27.36 KBedurenye
#17 interdiff-additional_uncaught-2521694-11-17.txt11.83 KBedurenye
#17 additional_uncaught-2521694-17.patch33.7 KBedurenye
#11 interdiff-additional_uncaught-2521694-8-11.txt405 bytesedurenye
#11 additional_uncaught-2521694-11.patch14.72 KBedurenye
#8 interdiff-additional_uncaught-2521694-5-8.txt10.97 KBedurenye
#8 additional_uncaught-2521694-8.patch14.76 KBedurenye
#5 interdiff-additional_uncaught-2521694-3-5.txt5.66 KBedurenye
#5 additional_uncaught-2521694-5.patch5.81 KBedurenye
#3 additional_uncaught-2521694-3.patch1.68 KBedurenye
#1 currency-fix_link_pattern_leading_slash-2521694-01-D8.patch828 bytesmarcusx

Comments

marcusx’s picture

marcusx commented
StatusFileSize
new currency-fix_link_pattern_leading_slash-2521694-01-D8.patch828 bytes

This fixes the problem in my case.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work 
+++ b/src/Entity/CurrencyLocale.php
@@ -34,10 +34,10 @@ use Drupal\Core\StringTranslation\StringTranslationTrait;
- *     "canonical" = "currency.currency_locale.edit",
- *     "create-form" = "currency.currency_locale.add",
- *     "edit-form" = "currency.currency_locale.edit",
- *     "delete-form" = "currency.currency_locale.delete"
+ *     "canonical" = "/currency.currency_locale.edit",
+ *     "create-form" = "/currency.currency_locale.add",
+ *     "edit-form" = "/currency.currency_locale.edit",
+ *     "delete-form" = "/currency.currency_locale.delete"

It's still wrong.

The value needs to be the link pattern, the path that's in those routes, not the route name.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new additional_uncaught-2521694-3.patch1.68 KB

Added the routes with the leading slashes in both files.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

canonical and edit-form used to have the same route, so they should also have the same path now.

Import is a custom form, so the add path should be used for create-form.

And we have another problem there. The link name needs to match the route name (edit-form => edit_form). Not sure if add is even a thing, Node for example doesn't have that.

Something that does exist is the canonical link template, however. It would be useful to add that, it should point to the list route (which then needs to be renamed to entity.currency.collection, including everything that points to it.

edurenye’s picture

edurenye commented
StatusFileSize
new additional_uncaught-2521694-5.patch5.81 KB
new interdiff-additional_uncaught-2521694-3-5.txt5.66 KB

It has some tests that doesn't pass, but I don't know if I should change the tests or is still something wrong with the routes.

LKS90’s picture

LKS90 commented

The changes to Currency.php and CurrencyLocale.php are not correct yet. They used to be route names, you changed them to a path. The idea would be to replace the old route with the new equivalent. Example:

OLD:

"canonical" = "currency.currency.edit",

NOW:

"canonical" = "/admin/config/regional/currency/{currency}",

SHOULD BE:

"canonical" = "entity.currency.edit_form"

You define route names in the routing.yml file and use those instead of full paths (unlike you currently do in Currency and CurrencyLocale). Your changes to routing.yml are not really correct either, you are changing where route names point:

The routes entity.currency.canonical and entity.currency.edit_form now point to the same path, that doesn't make sense.

Edit: Nearly forgot: change the status to Needs review when you want it reviewed/the patch tested.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That actually makes sense, because there's no way to view a currency, just edit it. So the workaround to still have a canonical link template defined is to use the same path. Not sure how well that works though.

canonical might not be needed at all. @Xano told me that he's on holiday until the 10th. Maybe we should wait for feedback from him.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new additional_uncaught-2521694-8.patch14.76 KB
new interdiff-additional_uncaught-2521694-5-8.txt10.97 KB

Now just fails one test, but I think it has nothing to do with routes.

Status: Needs review » Needs work

The last submitted patch, 8: additional_uncaught-2521694-8.patch, failed testing.

LKS90’s picture

LKS90 commented

The problem is in currency.links.task.yml. The base_route should be currency.amount_formatting, same as above in entity.currency_locale.collection.

The route you changed it to (entity.amount_formatting) doesn't exist in the routing file, so the tab isn't rendered and the test fails.

Edit: The correct currency.links.task.yml:

currency.amount_formatting:
  base_route: currency.amount_formatting
  route_name: currency.amount_formatting
  title: Formatting
  
entity.currency_locale.collection:
  base_route: currency.amount_formatting
  route_name: entity.currency_locale.collection
  title: Locales
edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new additional_uncaught-2521694-11.patch14.72 KB
new interdiff-additional_uncaught-2521694-8-11.txt405 bytes

Solved, now pass all the tests, I've changed this route by mistake, sorry.

LKS90’s picture

LKS90 commented
Status: Needs review » Reviewed & tested by the community

Yep, looks good now.

The last submitted patch, 1: currency-fix_link_pattern_leading_slash-2521694-01-D8.patch, failed testing.

The last submitted patch, 5: additional_uncaught-2521694-5.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Reviewed & tested by the community » Needs work

Thank you for picking this up!

+++ b/currency.routing.yml
@@ -32,7 +32,7 @@ currency.exchange_rate_provider.fixed_rates.edit:
+entity.currency.collection:

As far as I know, route names only have to prefixed with entity. if they map to link paths. The currency collection route is not a canonical URL and does not map to any of a currency entity's link paths.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

collection is a standard route/link template and renaming it makes sense, but the patch only does 90% of the work and doesn't actually add the link template.

collection is for example used by the default delete forms for the cancel/redirect destination.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new additional_uncaught-2521694-17.patch33.7 KB
new interdiff-additional_uncaught-2521694-11-17.txt11.83 KB

All the routes are changed now.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

I wasn't talking about collection at all, but about the entity route name prefix. As far as I know that's only supposed to be used for routes that map to entity link templates. The last two patches, however, add the prefix to routes that have nothing to do with entity link templates or specific entities. Maybe I missed a recent core change, so if so, could you please share a link?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

No, that patch was the wrong direction I think :)

I know you were. But collection *is* a link template, see NodeType for example. So renaming that is correct, but it only makes sense if we *also* add it as a link template.

So, back to #11 and just add the link template for collection. 

+++ b/src/Entity/Currency.php
@@ -35,10 +35,10 @@ use Drupal\currency\Usage;
  *   label = @Translation("Currency"),
  *   links = {
- *     "canonical" = "currency.currency.edit",
+ *     "canonical" = "entity.currency.collection",
  *     "create-form" = "currency.currency.add",
- *     "edit-form" = "currency.currency.edit",
- *     "delete-form" = "currency.currency.delete"
+ *     "edit-form" = "entity.currency.edit_form",
+ *     "delete-form" = "entity.currency.delete_form"
  *   }
  * )
  */

So the actual change that we are supposed to do here is now actually lost.

These need to be paths. Which it was until comment #5.

edurenye’s picture

edurenye commented
StatusFileSize
new additional_uncaught-2521694-20.patch27.36 KB
new interdiff-additional_uncaught-2521694-17-20.txt8.02 KB

So if I understand is like this, just editing to entity the link templates from the entities.
I think now it's ok, I didn't understand before.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new additional_uncaught-2521694-21.patch27.53 KB
new interdiff-additional_uncaught-2521694-20-21.txt1.68 KB

And also change those templates for the paths.

edurenye’s picture

edurenye commented
Related issues: +#2496897: Throw helpful exception if link templates are missing leading /

The last submitted patch, 17: additional_uncaught-2521694-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: additional_uncaught-2521694-21.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Title: Additional uncaught exception thrown while handling exception. (after enabling devel module) » Fix entity link templates and corresponding route names
Issue summary: View changes

So the root cause is really that the entity link templates are still route names instead of paths, and that the names of the corresponding routes do not always start with entity..

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

@edurenye: If you diff between branches to create patches, then you must rebase your feature branch after you've pulled the master branch. If you don't, all new changes will become part of the patch, which is what happened in #21.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new additional_uncaught-2521694-27.patch16.85 KB

Sorry, I always try to do that, but sometimes I forget.
I don't provide a interdiff as I did not change anything, just rebase.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Needs work

Overall this looks good. Thanks a lot for working on this!

  1. +++ b/currency.links.action.yml
@@ -1,24 +1,24 @@
+entity.currency.add:
+  route_name: entity.currency.add

    This route has no corresponding entity link template. 

  2. +++ b/currency.links.action.yml
@@ -1,24 +1,24 @@
+  route_name: entity.currency.import

    This route has no corresponding entity link template. 

  3. +++ b/currency.links.action.yml
@@ -1,24 +1,24 @@
+  route_name: entity.currency_locale.add

    This route has no corresponding entity link template. 

  4. +++ b/currency.links.action.yml
@@ -1,24 +1,24 @@
+  route_name: entity.currency_locale.import

    This route has no corresponding entity link template. 

  5. +++ b/currency.routing.yml
@@ -57,6 +57,14 @@ currency.currency.import:
+entity.currency.canonical:
+  path: '/admin/config/regional/currency/{currency}'
+  defaults:
+    _entity_form: 'currency.default'
+    _title_callback: \Drupal\currency\Controller\EditCurrency::title
+  requirements:
+    _entity_access: 'currency.update'
+

    This is an exact duplicate of the entity.currency.edit_form form route. We need to alias one to the other (is that even possible?), or drop one of the routes. 

  6. +++ b/currency.routing.yml
@@ -111,6 +119,14 @@ currency.currency_locale.import:
+entity.currency_locale.canonical:
+  path: '/admin/config/regional/currency-formatting/locale/{currency_locale}'
+  defaults:
+    _entity_form: 'currency_locale.default'
+    _title_callback: \Drupal\currency\Controller\EditCurrencyLocale::title
+  requirements:
+    _entity_access: 'currency_locale.update'
+

    This is an exact duplicate of the entity.currency_locale.edit_form form route. We need to alias one to the other (is that even possible?), or drop one of the routes. I checked several of core's config entity types and they don't provide canonical link templates.

To reiterate what @berdir and I said in previous comments: if we introduce routes that are prefixed with entity., these must be exposed as link templates.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@Xano: AFAIK, the reason for those duplicate routes is that you have duplicate link templates ( canonical and edit-form) that point to the same path. so I think we need duplicate routes so that urlInfo('edit-form/canonical') can resolve that to the correct route

Or remove canonical from the link templates.. core doesn't have those for config entitities anywhere AFAIK.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new fix_entity_link-2521694-30.patch13.45 KB
new interdiff-fix_entity_link-2521694-27-30.txt5.68 KB

Done.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

That's looking much better now. As a follow-up, we could try to use the default entity delete forms and save a bit of custom code...

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Reviewed & tested by the community » Fixed

There was something about the default deletion forms that I thoroughly hated, but that may have only been for content entities.

Thanks for working on this!

  • Xano committed 012c5bb on 8.x-3.x authored by edurenye 
    Issue #2521694 by edurenye, marcusx, Berdir: Fix entity link templates...

The last submitted patch, 20: additional_uncaught-2521694-20.patch, failed testing.

Status: Fixed » Closed (fixed)

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