Closed (fixed)
Project:
Redirect
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
23 Jun 2017 at 18:01 UTC
Updated:
9 Sep 2017 at 11:25 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
claudiu.cristeaPatch.
Comment #3
claudiu.cristeaIS
Comment #4
claudiu.cristeaBetter.
Comment #5
berdirI guess that makes sense, I think this was more or less directly ported from 7.x like this.
However, lets then explicitly use canonical for toUrl() as well, as for config entities, it can default to edit-form.
Also not convinced about moving the entity case out of the if. if there is no route to an entity, then it also won't have entity based redirects?
Also wondering if using predelete() would allow us to delete in bullk with only a single query for a batch of entities, but that's probably better done in a separate issue.
Comment #6
claudiu.cristea@Berdir, reading the docs from
EntityInterface::toUrl(), my understanding is that the URL follows this fallback cascade:link template (if exists) > uri callback (if is callable) > standard canonical (entity/{entity_type_id}/{entity_id})
but reading the code in
Entity::toUrl(), I don't see the latest fallback step. Or I'm missing something?Also it seems that we need to catch the exception because ::toUrl() doesn't return NULL/FALSE on failure.
Comment #7
berdirYes, there is no fallback, it throws an exception if the link template is not defined I think.
Catching exception shouldn't be necessary because we do check if the link template exists before.
Comment #8
claudiu.cristea@Berdir, then we exclude entities without link template but with
uri_callback.Comment #9
berdirYour patch does that already and I'm actually pretty fine with that.
Comment has a uri callback but it also has a canonical route, what forum is trying to do is completely broken and Item for example is an external url which shouldn't use the toUrl() system like that anyway IMHO, it would actually break the code here as we can't delete external redirects like that.
Comment #10
berdirYour patch does that already and I'm actually pretty fine with that.
Comment has a uri callback but it also has a canonical route, what forum is trying to do is completely broken and Item for example is an external url which shouldn't use the toUrl() system like that anyway IMHO, it would actually break the code here as we can't delete external redirects like that.
entity:node/1 is just an alternative way to route to the canonical route for node id 1, if there is no canonical link template then entity:$type/$id also can't work. See \Drupal\Core\Url::fromEntityUri
Comment #11
claudiu.cristeaOK.
Just a note: I think that redirects created by entities should be treated somehow as first-class-citizens and we need to implement a way to know that a certain redirect has a specific entity as dependency. I have this case: I'm migrating a D6 site to D8 and I want to have redirects in the new site for old URLs, including files/images. For this reason I have also this kind of redirects:
sites/default/files/d6/path/to/unmanaged/file.pdf > base:/sites/default/d8/path/to/managed/file.pdf
I'm expecting that this redirect will be automatically cleared when I'm deleting the managed file entity in D8 site. Now, I have to implement custom code to get these redirects removed on file entity removal. If we could mark a redirect being dependent on a specific entity then everything can be simplified and would allow to handle such cases, without custom code. We can, for instance, store also the entity type ID and entity ID in the {redirect} table (and leave blank for non-entity redirects). Or even store them in a new {redirect_entity} table. A new table would not impact performance because it would be used only on creation or deletion of redirects.
Comment #12
berdirThanks. Yeah, file entities are special/weird, we could add special support for them, but it will always need to be a special I think because the file URL isn't the canonical entity URL.