🇪🇸Spain @marcoscano

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.

Thanks all!
Merged and tagged https://www.drupal.org/project/entity_usage/releases/8.x-2.0-beta19 → with this.

Yeah this will happen on sites where the build workflow doesn't start with a drush cache:rebuild.
I'll put together a MR to try to avoid that.

Thank you for filing this and for contributing.
I am very reluctant to add more code and special-cases to the list controller, which is already somewhat messy code special-casing paragraphs and blocks.
I'd rather we can wait until ✨ Issue Event for Row Rendering Active lands, then other modules can alter the list in any way they want.
Along these lines, I also believe the new plugin and settings form validation code should be part of the "Paragraphs entity embed" contrib module, how about we move this ticket into their issue queue?

Can you please re-roll the MR with latest changes from -dev? I am still seeing unrelated changes there, but maybe that's due to the merge conflicts.
Also, we'd need tests to check the new functionality being added.
Thanks!

Can you please re-roll the MR and add tests for the new functionality?

Thanks!

Argh right! Sorry I went too fast on these return type changes...
Thanks for fixing them and the update test 🙏
Merged! Will tag a release in a little bit.

Thanks for contributing!

Welcome aboard! I just granted @ghost of drupal past co-maintainership status on the project.
Technically, the project isn't "fully abandoned", but it's true that I have rolled off of the project that was using it, and neglected keeping an eye on the issue queue.
Thanks for helping out! 👍

OK, fixing all issues on level 6 is more work than I anticipated, so I'm adding a bunch of things to the baseline. We will be at least enforcing more stricty-typed code going forward. PHPStan is happy now, but in the process I seem to have broken a few tests.

Unassigning myself from this ticket for now since I need to switch to other things, but I'll revisit this later in the week if it's still at the same place.

@alexpott thanks for opening and for the initial branch.

I'll dedicate some time to this one today.

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

Fair enough 👍
I was mainly considering the use-case of a redirect being created under the hood on node save, but it's true the problem goes beyond that and lies on the fact that the identifier we are using to track a relationship (path alias) can be changed outside of the source node form, and this can potentially break the relationship tracked.

I have opened 📌 Consider updating usage when path aliases change Active so we can explore this problem in more detail. I agree it's going to be complex, so if we find it's almost unfixable in a reasonable way I'd also be OK to rescope that issue to implement workarounds to mitigate the issue as much as possible.

Thanks again, this is a great win to the module! 👏

marcoscano → created an issue.

@alexpott Awesome work! Thank you very much for working on this. I agree things will get much cleaner and easier to modify going further.
I was not paying too much attention to redirects so far since in my mind for some reason it wasn't "clicking" that we can track them just as first-class entities :)

I have left a few minor suggestions / nitpicks as comments in the MR, but apart from that I believe there are 2 things we should still do here:

1) Implicitly support redirect basefields

I know #3326567: Track some base fields → is asking to be able to granularly define which basefields we track and if we end up doing that, it could solve this, but to me there is no point in selecting redirects as source/target and not wanting to track basefields, so I would keep the two issues separate, and just special-case redirect basefields in this MR. If a site-builder enabled redirects as source/target, then we track its basefields regardless of the global checkbox. I think we can add some logic in \Drupal\entity_usage\EntityUsageTrackBase::getReferencingFields() to bypass the global setting for now if the entity is a redirect. (I also have reservations to the other issue, not sure how wild the form could get in order to accomplish that).

(A quick aside, I believe a UX win we could consider sneaking into this same MR (although I don't feel too strongly if you prefer not) is to add states to the settings form, so that whenever redirect is enabled as source, we also enable it as target, and vice-versa. I see no point in selecting just one of them.)

2) Do not display redirect entities in the UI

Much like we special-case paragraphs and blocks when we display usage info on the UI, I believe we should do the same with redirects, which are just "bridges" to the other entities and aren't useful as rows in the usage page. It's unfortunate because \Drupal\entity_usage\Controller\ListUsageController::getRows() is getting more and more complex with yet another special case, but I think that's what we have to do here...

An example of the issue users would find if we don't special-case them:

- Create node Foo with path /foo
- Create node Source1 with a link in body pointing to /foo, save node
- Edit node Foo changing its alias /foo to /bar. (this creates a redirect from /foo to /bar)
- Go to view the Usage page of Foo. It now displays 2 rows:
- Source1 (correct)
- Redirect entity (maybe we can remove this to avoid confusion?)
- (note: At this point in DB we only have registered redirect as a source, since Source1 hasn't been processed)
- Save node Source1 with no changes
- View usages of Foo again:
- Source1 (wrongly displays "Used in Old revision(s)")
- Redirect entity (could be removed too)
- (in DB we have now registered the redirect both as source and target)

In my mind, the ideal scenario would be that we should either transparently display the source entity pointing to the redirect, or do something like we do with paragraphs, and indicate somehow that there is a redirect between source node and target node. Displaying a redirect in its own row, even if technically correct, would confuse editors IMO.

What do you think?

Thanks again!

Excellent! Thank you for contributing! 🙏

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

Thanks for jumping in and for the generic solution in the base class 👍

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

Yeah the content -> files list isn't something provided by this module. I'm closing this out for the time being, but feel free to re-open if you see that there is unexpected behavior in the functionality provided by entity_usage.

Thank you! Love the simplification that comes with ::checkAndPrepareEntityIds() 👍

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

Agreed, thank you! 👍

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

On a fresh install, a file uploaded as an Image media item reports that it has 2 usages by that media item

This sounds like a bug. Could you please try with the latest -dev, and indicate steps to replicate the problem on a fresh install?

Thanks,

Note that I've tested the code and there looks to be test coverage too but the massive site I'm working on does not use this plugin so I've not done a full performance test.

Yes, we do have test coverage, and the changes won't make performance worse IMO, so let's go with it.

Thanks!

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

Great, let's go with this. Thank you! 👍

👍 thanks!

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

Yeah I was testing through the UI, and admittedly my environment might have been polluted by different testing in different branches, stopping, resuming, etc. Paragraph total numbers were still increasing as the batch progressed, for some reason.
I nuked everything, and in a fresh install, I can no longer reproduce, so for now I'm blaming my testing which probably wasn't accurate enough.
Thanks for checking!

Pushed a quick update function to clean up remaining items from the queue, if existing. Was that what you had in mind?

Thanks!

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

Merged! Thanks again! 👏

@alexpott thanks for working on this one! I left a question on the MR (to which I suspect the answer, but just in case). Apart from that, this is good to go as well IMO 👍

marcoscano → created an issue.

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

Looks good to me, thanks!

This is indeed a great idea. Thanks for contributing! 👍

  /**
   * The number of IDs to load when in bulk mode.
   */
  const BULK_ID_LOAD = 1000000;

@alexpott in you testing, did you see a significant impact in changing this value? I did a quick check in my local and it does seem to have a meaningful change in how fast the batch processes, so I am wondering if it makes sense to make this configurable. I don't think we need to expose this on the UI, but a settings value that people could define/override per environment (falling back to the constant) seems to make sense for me. In any case, if that's the case we can make that as an improvement in a follow-up.

For now this looks good to go from me. Thanks again!

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

Sorry, I believe the edit  https://www.drupal.org/node/2983971/revisions/view/11359926/13865806 → changed substantially the nature of the text. Admittedly, it wasn't the most clear and helpful documentation page, but it was meant to point users to places where they could be informed about the issues and challenges when setting private access on media items. The new text (along with the "No known problems" tag added) conveys the message that it's easy to do and there are no risks, which IMO is somewhat misleading.

I added more context, hopefully now it makes it clearer for everyone.

Thanks!

Looks good to me! Pushed a tiny cleanup in the inline comment of the test that was no longer relevant, and merged 👍
Thanks!

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

Looks great, thanks! 👍 I actually intended to fail CI on these too but missed turning it on in the other issue :)

The only slightly controversial change is protected function summariseRevisionGroup() to protected function summarizeRevisionGroup(). I think this is okay because the project is still in beta and also this is a protected method.

I don't feel this is a problem at all. Thanks for bringing it up!

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

Congratulations Adam! Well deserved! 👏

Thanks for contributing! 👍