Barcelona, Spain
Account created on 26 April 2011, almost 14 years ago
#

Merge Requests

More

Recent comments

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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.

🇪🇸Spain marcoscano Barcelona, Spain

@alexpott thanks for opening and for the initial branch.

I'll dedicate some time to this one today.

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Excellent! Thank you for contributing! 🙏

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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.

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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,

🇪🇸Spain marcoscano Barcelona, Spain

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!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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!

🇪🇸Spain marcoscano Barcelona, Spain

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

Thanks!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Merged! Thanks again! 👏

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Congratulations Adam! Well deserved! 👏

🇪🇸Spain marcoscano Barcelona, Spain

Re-rolled and fixed merge conflicts

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Nice work! Thanks for helping!

I have opened 📌 Allow users to clear the bulk inserting state flag on the UI Active as a follow-up so we can add an option to clear the state flag on the UI, but that isn't a huge deal IMO.

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Thanks for contributing!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Thank you @alexpott for helping with making the module better!

🇪🇸Spain marcoscano Barcelona, Spain

Nice, thanks for helping out! 👍

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Thank you for filing and fixing it! :)

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

Sorry I don't believe this is a need common enough to justify including it in this route. This controller is already too complex, and I'd rather we keep it as lean as possible (considering it already has a lot).
If others believe this would be useful for their projects feel free to add comments here. In the meantime you can use the patch or patch it locally in your project.
Thanks in any case for contributing!

🇪🇸Spain marcoscano Barcelona, Spain

Wut! I hadn't realized Linkit for Link field Fixed got in, actually quite a while ago... :)
Since our current plugin was meant to support the original usage of Linkit (embedded links in wysiwyg), let's adjust the \Drupal\entity_usage\Plugin\EntityUsage\Track\Link plugin to also support the format their widget is using. I believe this is the easiest and cleanest way to support this.
Thanks!

🇪🇸Spain marcoscano Barcelona, Spain

I am OK with the idea of simplifying the UI, but this controller is getting more and more convoluted to make everyone happy :)
After Differentiate pending/old revisions and default translations Active , there's more code dealing with these rows/columns, so we'll want to make sure we have tests that cover all possible scenarios.

🇪🇸Spain marcoscano Barcelona, Spain

EntityUsageListTrait is not being used anywhere inside ListUsageController.php... how is this a fix for the issue? 🤔
If drush updb was failing, all sites updating this module would have faced this problem. The fact that there are no other reports (I also didn't find it when I upgraded the sites I maintain) could suggest there is another issue interfering in your scenario? If we could have more details to troubleshoot it would be good (eg full stack trace, steps to reproduce on a clean install, etc).

Thanks!

🇪🇸Spain marcoscano Barcelona, Spain

Thanks for helping! This is ready to go but needs a re-roll

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

I agree with the idea behind this, but I see a challenge in "filtering out the noise" on the listing page, because it could open the door to a scenario where the service reports usages of an entity (for example in the warning message when deleting an entity), and when editors go to the usage page to check what other entities reference it, they find nothing because the only usages are in past revisions. We'd need to find a way to let them know "there is nothing here because your site admin decided not to display past revisions" or something like that.

🇪🇸Spain marcoscano Barcelona, Spain

Tagged a new release with this 👍

🇪🇸Spain marcoscano Barcelona, Spain

Looks good to me, thanks both for contributing!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

If you end up implementing it as a block based on Type Tray's categories, I think a submodule like type_tray_block or something like that makes total sense! 👍

Thanks!

🇪🇸Spain marcoscano Barcelona, Spain

and we moved all files and placed in site/oursite/files/{file_name]

Are the files managed in Drupal or these are just links pointing to files in disk that happen to be at these locations? If the files are not file entities in Drupal (ie in the file_managed table), then this module won't track them, by design.

🇪🇸Spain marcoscano Barcelona, Spain

Happy to get this in. Does anybody feel like proposing a Merge Request?
(just clarifying that the proposed resolution in the ticket description has it backwards... :) if there is a field on the node that can reference media entities, then the node is the source, and media is the target. In other words, the field is always attached to the source, and what users reference is always the target)

Thanks!

🇪🇸Spain marcoscano Barcelona, Spain

Thanks for filing this and for the fix.
Is there a scenario where old URLs should be supported too? In other words, should be change the regex to the new format or should we just accept the new format as well?

🇪🇸Spain marcoscano Barcelona, Spain

Fixed, thanks!

🇪🇸Spain marcoscano Barcelona, Spain

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

🇪🇸Spain marcoscano Barcelona, Spain

+1 for removing. Folks that know how to fix it probably don't need the reminder.

🇪🇸Spain marcoscano Barcelona, Spain

I agree that the module could handle the misconfiguration in a more graceful way. Merge Requests are welcome if anyone feels like working on it.

🇪🇸Spain marcoscano Barcelona, Spain

Can we please open a new ticket for this and create the MR there?
@justcaldwell it would be great if you could compare the current fix with this new proposal and see if it solves the performance impact you are experiencing? Thanks!

🇪🇸Spain marcoscano Barcelona, Spain

Thank you both, I think the approach that casts the variable to a string is simpler and more readable.

🇪🇸Spain marcoscano Barcelona, Spain

Thanks for contributing!

🇪🇸Spain marcoscano Barcelona, Spain

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

Production build 0.71.5 2024