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! 👍
marcoscano → created an issue.
Re-rolled and fixed merge conflicts
marcoscano → made their first commit to this issue’s fork.
👍 thanks!
marcoscano → made their first commit to this issue’s fork.
Nice improvements, thank you!
marcoscano → made their first commit to this issue’s fork.
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.
marcoscano → made their first commit to this issue’s fork.
marcoscano → created an issue.
Thanks for contributing!
marcoscano → made their first commit to this issue’s fork.
Thank you @alexpott for helping with making the module better!
Nice, thanks for helping out! 👍
marcoscano → made their first commit to this issue’s fork.
marcoscano → made their first commit to this issue’s fork.
Thank you for filing and fixing it! :)
marcoscano → made their first commit to this issue’s fork.
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!
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!
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.
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!
Committed. Thanks all!
Thanks for helping! This is ready to go but needs a re-roll
Committed, thank you! 🙏
marcoscano → made their first commit to this issue’s fork.
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.
Tagged a new release with this 👍
Looks good to me, thanks both for contributing!
Thanks for contributing!
marcoscano → made their first commit to this issue’s fork.
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!
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.
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!
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?
Fixed, thanks!
marcoscano → made their first commit to this issue’s fork.
+1 for removing. Folks that know how to fix it probably don't need the reminder.
dan2k3k4 → credited marcoscano → .
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.
Looks good to me! Thanks for the quick fix!
marcoscano → made their first commit to this issue’s fork.
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!
Looks good to me, thanks for contributing! 👍
marcoscano → changed the visibility of the branch 1.0.x to hidden.
marcoscano → made their first commit to this issue’s fork.
Looks good to me, thanks for contributing! 👍
marcoscano → made their first commit to this issue’s fork.
Looks good to me, thank you for contributing!
marcoscano → made their first commit to this issue’s fork.
Thanks! :)
Thank you both, I think the approach that casts the variable to a string is simpler and more readable.
Thanks for contributing!
marcoscano → made their first commit to this issue’s fork.