🇪🇸Spain @marcoscano

Thanks @mmenavas! Looks great 👍
I went ahead and created https://www.drupal.org/project/url_friendly_options/releases/2.0.0 → after merging.

Thanks for raising this up, it had felt outside of my radar.
In recent work we have added support for redirect and as part of that we realized it didn't make much sense to expose redirects to be used as source but not track redirect basefields, since that's the only way redirects can be linked to other entities. Because of that now if redirects are selected as source/target, we disregard the option to track basefields and also track redirects since that's the intended most specific behavior.

I would be happy to move forward the same special-casing for menu links, since that's apparently the same scenario?

Support for redirects has been implemented recently in 💬 Entity Usage when a URL Redirect Exists Active and the UI displaying usages was improved in 📌 Consider updating usage when path aliases change Active . Note that changing a path alias / creating a redirect outside of the affected entity's form makes it challenging to update all places in the system that could be using the old URL. Due to this, when there is a need to re-calculate tracking based on a URL that changed, we use a queue to update the corresponding usage information. This means that the system won't be really "up-to-date" until the next time you run cron and all queued items have been processed. (This may explain why you see differences in behavior after you create the redirect and after you run drush to re-generate statistics... the second action would be equivalent of running cron (or waiting a bit if you have cron scheduled to execute in background)).

Does this clarify things a little bit?

I'm thinking that if we go the route of identifying Trash is enabled in the usage controller in 📌 Document interaction with the Trash module Active we will likely be able to fix this as part of that issue too?

@bkosborne sorry just re-read the issue summary and I am realizing you had already suggested something along those lines...

An alternative approach would be if this module detected if the Trash module was enabled, then it could deactivate trash's hiding feature on the usage page to it continues to show that usage, but then also indicates that the source entity is in the trash. All of that adds a pretty big coupling to the Trash module though.

So yes I believe we should go this route, even though I think it's suboptimal from a maintenance perspective... 🤷‍♂️

I looked a bit deeper and I see that there seems to be a way to execute code bypassing Trash's hiding mechanisms. I don't really love the idea of special-casing Trash's behavior in our controller, but at this point maybe it's a good compromise?
So instead of having:

        $source_entity = $type_storage->load($source_id);
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

we could have something along the lines of (untested):

        // Special-case the Trash module behavior, which hides the entities in
        // trash from all entity_loads.
        if (\Drupal::getContainer()->has('trash.manager')) {
          $trash_manager = \Drupal::getContainer()->get('trash.manager');
          $source_entity = $trash_manager->executeInTrashContext('inactive', function () use ($type_storage, $source_id) {
            return $type_storage->load($source_id);
          });
        }
        else {
          $source_entity = $type_storage->load($source_id);
        }
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

and similarly to other places we may be checking things on the source entities (for example access checks, getting the labels, etc). If we know the entity is in trash, we may even add a suffix of type " (in trash)" to the label...

Thoughts?

Fixed in 🐛 Exception thrown on usage page if a layout builder block has usage but its parent entity was deleted Active
Thanks!

Thanks for raising this, and for contributing the fix.
I agree on the approach, there are downsides as you mentioned but we are not making anything worse with this fix.
Also thanks for fixing 🐛 Entities used by content blocks should not use label or status of layout builder node unless they are "inline" (non-reusable) Active which I agree makes sense to fix here 👍

marcoscano → made their first commit to this issue’s fork.

That's odd, I can't really reproduce it on a new installation. The first output you pasted in the issue summary made me think the code was being upgraded instead of installed for the first time.
Do you have anything particular about your project that may be interfering?

Thanks for working on this! I left a review, nothing super blocking, but it would be good to check if we can improve the test without the waiting calls.
Also, not sure if I'm seeing things right, but maybe the MR is being targeted against 1.x (instead of the 2.x new branch)
Thanks!

Ouch, thanks for raising this. It's indeed very unfortunate that the Trash module is so aggressive when hiding trashed entities... This means that our code in the controller:

        $source_entity = $type_storage->load($source_id);
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

will just think the source entity is broken since it can't be loaded, and skip it entirely from the table. I actually think this behavior from the Trash module could produce other unintended effects on sites using that module, because "the entity was moved into the trash" doesn't necessarily mean IMO "no other piece of code should be able to load this entity".

I'd like to give this a little more thought, but at this point I'm leaning towards just displaying a general warning message at the top of the usage page if the Trash module is enabled, or something along those lines. Thoughts?

Good catch, this is indeed a bug.
Thanks for contributing!

After upgrading code to newer versions, you always need to run drush updatedb before doing anything else. Can you confirm if doing this after bumping to the new version fixes the issue for you?
Or alternatively, can you reproduce this in an installation where this module has never been enabled?
Thanks!

marcoscano → made their first commit to this issue’s fork.

Sorry for the delay. This has been fixed in 🐛 InvalidArgumentException: The user-entered string must begin with a '/', '?', or '#'. Active

Sounds good to me, thanks for contributing! 👍

marcoscano → made their first commit to this issue’s fork.

Yes, this would benefit all sites that are using nodes as some sort of "microcontent" that have no meaningful representation standalone.
I'm happy to move this forward if folks are willing to start a MR + tests.
Thanks!

Let's go with the identified fix for now, please open a new one with steps to reproduce if there is something else to be fixed here.
Thanks all!

Thanks for contributing!

marcoscano → made their first commit to this issue’s fork.

Sorry for the late reply. It would be great if someone could submit this in the form of a GitLab MR, reroll with current -dev, and include the new #media template variable that was added in ✨ Add Media Entity To All Templates Active
Thanks!

Sorry for the late reply. It would be great if someone could submit this in the form of a GitLab MR, reroll with current -dev, and include the new #media template variable that was added in ✨ Add Media Entity To All Templates Active
Thanks!

I am not too familiar with videoask, but this PR is basically just allowing to embed anything from any URL, which defeats a little bit the purpose of the per-plugin providers architecture. If we could restrict the URLs accepted to something that's unique to this provider, it would be better.
Thanks!

Here too, let's clean up the code and include the new template variable added in ✨ Add Media Entity To All Templates Active , then this is good to go.
Thanks!

Sorry for the late reply.
I'm OK adding this provider, if you can please just clean up the theme variables, and make sure to include the new #media added in ✨ Add Media Entity To All Templates Active , we can get this in.
Thanks!

👍 Sounds good to me. Thanks for contributing!

marcoscano → made their first commit to this issue’s fork.

Sorry for the late reply. Committed, thanks! 👍

I have spent some time looking into this today, and I too, am unable to reproduce it.
@wilco please update testing instructions on a fresh install if you can, or test if the MR provided solves the issue for your particular case.
Also, thanks @dave reid for helping out! 👍

Do you have redirects enabled as souce/target in the settings page?
As soon as you have a redirect in the middle it starts becoming one extra entity in the chain, so that needs to be tracked as a separate entity.

👍 nice! Thanks for helping!

marcoscano → made their first commit to this issue’s fork.

I am not sure if we should avoid the empty string in the first place, but for now just replacing the !isset() with empty() solves the bug for me.

marcoscano → created an issue.

Hi @seanb! I am sorry this is causing issues for you.

Maybe @alexpott can chime in with more context if needed, but my recollection is that:
- The queue batch manager was a part of the codebase that had been added in the past as a stopgap solution to the poor performance when regenerating all info. It had been added without test coverage (sorry my bad), and had significant duplication with the normal batch manager
- In the past few months Alex worked on a lot of great improvements to the performance of the module in general, and in particular to the batch regeneration process

With the recent performance improvements (and knowing they are incompatible with how the queue works), it felt unnecessary to keep maintaining both batch managers, especially since they aren't particularly small/trivial. We felt the performance gains of the bulk insert batch manager were enough to justify having the Drush command always work as a standard batch operation, without the need to offer a queue mode as well.

Just tagged a new release with this and a couple other bug fixes.

Great, thanks! 🙏

Thanks for contributing!

marcoscano → made their first commit to this issue’s fork.

Sounds good to me. Thanks for contributing!

marcoscano → made their first commit to this issue’s fork.

@alexpott I am happy with the MR, but see the status is still NW. Did you want to include anything else or is this ready to go?
Thanks!

Thanks for contributing!
I have merged the PR, and tweaked the project page to include some end-user scenarios that include some of the terms you mentioned above.
I still think the relationship methods list is important since that may be very relevant to developers reading the project page, so they can know the extent of the possibilities of the module.

Thanks again!

Thanks for contributing!

marcoscano → made their first commit to this issue’s fork.

You may be using a quite old version of the codebase. That variable no longer appears anywhere in `-beta23`.
Can you try with the most recent release and see if that's still an issue?

Thanks for pointing this out. True, the text on the project page is likely inherited from when the project was just an idea to expand what core's file_usage does... :)

Would you be up for creating a first pass at refactoring the page + readme file?

Thanks!

Sounds good to me. 👍
Clarifying the title though, since it was a bit misleading. The bug wasn't about the module tracking usage on types that were not set to, but it was collecting potential URL changes on all entities during presave, instead of collecting these URLs only on the ones that interest us (the ones marked as source). There was no functional bug, it was just a performance issue.

Thanks for reporting. Fixed.

marcoscano → made their first commit to this issue’s fork.

marcoscano → created an issue.

Thanks for reporting, just fixed this in 🐛 Updating to 8.x-2.0-beta22 causes drush updb to fail Active and tagged -beta23 with the fix. Can you please try with that?
Thanks!

Thanks for reporting. Yes there's a bug there in scenarios where the entity type no longer exists.
I've just committed a fix, will tag a new release shortly.
Thanks!

Good catch, committed!

Hi all, thanks for contributing.
This module has tests, but unfortunately GitLabCI hasn't been configured yet, so we can't see the tests running, and as a consequence, we can't be sure the only change needed for D11 is the one this MR implements.
Can we please set up GitLab CI → in this same MR?
Thanks!

Yes! Let's fix that on https://www.drupal.org/project/pate/issues/3467514 📌 Drupal 11 compatibility fixes Active

I agree it's a lot of code but definitely the most flexible fix. Thanks for writing it!
I have fixed a minor typo on commit.

marcoscano → made their first commit to this issue’s fork.

nod_ → credited marcoscano → .

Hi there! My concerns from 📌 Drupal 9/10 compatible release and plan? Fixed are still more or less the same.
If people want to keep exploring / testing the module in D11, I'm happy to merge a MR that adds compatibility, feel free to open one.
As for a stable release, we'd need to make sure the list access works well before going that route.
Thanks!

OK, thanks for confirming 👍
I've tagged -beta21 with this fix.

Thank you @alexpott!

I have merged the MR since it's harmless and might help others.

@sirclickalot could you please test if this solves the issue for you?

Thanks,

I can't really reproduce this myself, are you able to provide the full stack trace to see if we can understand better what's going on?

Thanks!

Thanks for contributing. I am OK with the fix, but we'd need test coverage to make sure we don't regress this in the future.
Thanks!

Thank you!

marcoscano → made their first commit to this issue’s fork.

Thanks for contributing the fix. Can we also add a test to prevent this won't regress in the future?
Thanks!

Sounds good to me, thanks for contributing!

marcoscano → made their first commit to this issue’s fork.

Excellent, thanks again! 👏

@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!

Yeah, that return; inside the foreach ($fields as $field_name) { makes zero sense, and my memory can't bring any light into why that ended up there... Nice catch, thank you!

marcoscano → made their first commit to this issue’s fork.

Excellent, thank you!

marcoscano → made their first commit to this issue’s fork.

@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. 👍

@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?

Excellent, thanks for contributing!

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!

Thank you! 👍

marcoscano → made their first commit to this issue’s fork.

Thank you for contributing, let's please fix this as part of 📌 Clean up and improve CI jobs Active .

marcoscano → created an issue.

Thanks for the fix!
I personally don't think we need to expose the icon/thumbnail paths to be translatable. If we find people need it, we can always change that later.

marcoscano → made their first commit to this issue’s fork.

Thanks for reporting and for contributing a fix. I agree that users that can access this field can already take over the site so this can be fixed in public.

Merged!

marcoscano → made their first commit to this issue’s fork.

marcoscano → created an issue.

Thanks for working on this.
Technically, we still need to test that in a bundle-less entity type, nothing breaks and the text displayed is what we expect.
Also, since we are here... can we please update the bundle label in the test module to be different than the bundle id? This way we also test that we are displaying on the UI the label, and not the ID.