The current way the usage table is rendered is not easily modifiable, and if multiple patches are needed (to provide customizations of how each row is output), it can quickly become unwieldy.
N/A
Introduce a new event, `EntityUsageRowRenderEvent`, that gets dispatched to determine the values used for each row. This would allow other modules to override things like the labels or link to the entity without the need to patch the module.
This might be a way to implement part of the refactoring task
N/A
Introduces a new event
N/A
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
bkosborneComment #4
bkosborneI think this approach makes sense. I'm not sure if it's worth having the module itself provide the "starter" event subsubscriber to provide all the initial data for the row or not. I don't think it's a super common pattern to do that, rather the module would still have its core business logic present to get those starter values, then dispatch an event with those values to allow other modules to alter them. Though, this method does make it easier for some other module to completely disable the main event subscriber if they want.
Are the module maintainers on board with an approach like this? If so, maybe we can get some tests written.
Comment #5
partyka commentedI agree that it's not super common, but the reason I chose to do this here was flexibility -- like you said to disable the main subscriber if desired. You could also disable the main event subscriber and override the "starter" subscriber if your use case allows for it.
Comment #6
marcoscanoThank you for creating this and for the MR.
I think it's OK to open an integration point and allow modules to alter the row contents. However, as mentioned above, I would rather have the current code do its thing and just dispatch the event (or even a good ol' alter hook, it works fine for me), so other modules can alter the contents of the generated row. I could see the most common use for this would be to tweak things a bit, not re-writing everything entirely. (maybe if projects need to do something completely different it would be easier to just take over the controller at that point?)
In my mind with this new hook/event there would be a small performance penalty to all sites, for the benefit of the ones that do need to alter something, but I believe the trade-off is worth it.
Thanks,
Comment #7
partyka commentedAgreed .. I just thought that it made the most sense to provide the module's default event implementation using the same mechanism provided to anyone else who might be interested. Of course, the original way works too and I can see merit to that as often there's just a tweak needed by a consuming module. FWIW, doing it this way helped me identify an appropriate place to fire the event.
Just please let me know if, after considering this, you'd still like to have the generated row implementation in the controller.
Comment #8
marcoscanoYes, I think this would be the preferred approach, for maintainability reasons at this point. Thanks! 👍
Comment #9
partyka commentedComment #10
marcoscano@partyka thanks for working on this. The MR is still marked as Draft, can you please remove that when it's ready for review?
(also, there seems to be a lot of changes in the controller code that may not be related to this change? It would be great if we could remove those to make reviewing easier)
Thanks!
Comment #11
partyka commented@marcoscano
I've reverted (most of) the unrelated changes. I just left in a couple of line-breaks introduced when I ran it through PhpStorm's drupal-standards formatter. I can certainly revert those too if you'd like.
I've removed the draft status from the MR.
Comment #12
partyka commentedComment #15
marcoscanoCan 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!
Comment #16
cgmonroe commentedHaven't had a chance to review this yet bu just want to make sure this use case is covered:
Have a new plugin to handle embedded paragraphs / paragraph. Embedded paragraphs can be used in multiple places.
Creating usage table for an entity used in a paragraph that is used in an embedded paragraph which has been used on two different nodes.
The nodes are the only directly editable place the entity used by the paragraph can be removed. Knowing it's source is a paragraph is useless to content editors. They need an edit link.
The event needs to be able to:
This means that the event needs to also include the raw info needed to do this. At a minimum, the target id and type info.