Consider updating usage when path aliases change

Everything is working now. So worth reviewing - I will update the issue summary outlining the solution. Need to test this with linkit too just to see how that is or is not affected.

Yep linkit links are not affected by this problem because when you use them you get links like:

    <a href="/node/1" data-entity-type="node" data-entity-uuid="90bbdd6c-4cac-4651-bb48-81ea05df7c23" data-entity-substitution="canonical">Linkit link</a>

And the UUID does not change when the alias does so this is good. For that reason I think it is only html_link that is affected by this. Ahhh... there is one more kinda odd place - the Link plugin does:

    /** @var \Drupal\link\LinkItemInterface $link */
    if ($link->isExternal()) {
      $url = $link->getUrl()->toString();
      $entity_info = $this->urlToEntity->findEntityIdByUrl($url);
    }
    else {
      $url = $link->getUrl();
      $entity_info = $this->urlToEntity->findEntityIdByRoutedUrl($url);
    }

So potentially that could be affected by putting external urls to yourself is an odd usages of the link plugin. On the other hand recalculating the usage for links is quite quick. Maybe this we could fix this in a follow-up. Not sure.

Added detail to the issue summary.

Nice work, thanks! 👏

I left a few comments inline, mostly nitpicks, perhaps the most relevant is to wether we need another interface + trait or if we can just add a new method to the base class.

Re:

So potentially that could be affected by putting external urls to yourself is an odd usages of the link plugin. On the other hand recalculating the usage for links is quite quick. Maybe this we could fix this in a follow-up. Not sure.

I don't feel strongly either way, it does feel like an edge-case that will affect a small number of sites, and can be fixed in a follow-up, if necessary. If you want to give it a try and see it doesn't add too much more work to this and wants to sneak it in, I'm OK too 👍

I have only reviewed the code, tomorrow I'll try to set some time to play with this manually, but for now, looks good!

Thanks again!

@marcoscano responded / fixed most of the review points - thanks again for the great review. Left a couple of questions on the MR for you :)

@alexpott Another follow-up I think we may want after this is to improve the settings form.

The redirect setting redirect.settings:auto_redirect is TRUE by default on module install, and almost all sites have it enabled. However, we don't track redirects by default, so most sites will not know they need to check redirects as source/targets to actually get the full chain tracked when editors change node aliases and redirects are created under the hood.

I believe it would make sense to create a follow-up to:
- Display a warning message in the settings form if redirect.settings:auto_redirect is TRUE and redirects are not enabled as source/target
- Add states to the checkboxes so you can only choose both source and target at the same time for redirects, since it doesn't make sense to have only one of them enabled.

What do you think?

@marcoscano I think we do track redirects by default - they're content entities - class Redirect extends ContentEntityBase {

@marcoscano I think we do track redirects by default - they're content entities - class Redirect extends ContentEntityBase {

True! 🤦🏻 Sorry I got confused for a bit during the tests I was performing locally.

But actually the fact that I got confused could be an indicator that others could too... :) Maybe it still makes sense to have some sort of help for people indicating "since your site creates redirects under the hood when aliases change, you might want to select the redirect checkboxes here". I believe a good number of sites might remove most of the default source/target pre-selected entities to reduce noise / improve performance, and uncheck redirects without realizing the consequences of doing so.

In any case, this is definitely material for a different issue, if any. 👍

I agree that we could improve the UI to make it easier to set things up and provide sensible defaults for this stuff. I'll create an issue to do explore UI improvements.

@alexpott Nice! Thanks, I like the new approach, inline with the current way of updating the usage on existing entities. I agree that there's now some level of duplication between ::recreateTrackingDataForField() and ::trackOnEntityUpdate(), but I'm OK cleaning that up / optimizing in a follow-up, to minimize the scope of this one.

I left a couple minor suggestions on the MR, and after that this is good to go IMO.

Thanks!

I've removed the duplicated code in EntityUsageTrackBase::trackOnEntityUpdate() and EntityUsageTrackBase::recreateTrackingDataForField() for three reasons.

1. It fixes a bug - recalculating the usage from $entity->original assumes that the results will be the same as before but this is not necessarily the case because of how path aliases work.
2. It's more performant to select the rows from entity_usage than to recalculate
3. It informs what the new method should be called - it's now EntityUsageTrackBase::updateTrackingDataForField() which is better for both use-cases