The ed11y_page table contains incorrect or missing entity_id values, incorrect entity_type and incomplete page_path values.
This probably doesn't matter for most people, but I'm trying to build some custom views that display editoria11y data related to groups (group module), and because the entity_id and entity_type are wrong, adding the relationship is currently impossible.
For example, this is what I see in the ed11y_page table:
The actual correct data for these entities is:
I believe this all stems from editoria11y_page_attachments() in editoria11y.module.
This bit conflates entity types with bundles, which are two different things:
$entity = $candidate;
// Get entity type label via the bundle entity (works for any
// content entity type, not just nodes).
$bundle_type_id = $entity->getEntityType()->getBundleEntityType();
if ($bundle_type_id) {
$bundle_storage = Drupal::entityTypeManager()->getStorage($bundle_type_id);
if ($bundle_storage) {
$bundle_entity = $bundle_storage->load($entity->bundle());
if ($bundle_entity) {
$entity_type = $bundle_entity->label();
}
}
}
In my case it's the different between getting "group" which is an entity type and getting "About", which is one of several group entity bundles.
I'm not sure yet how the entity_id and page_path are wrong/incomplete. I suspect the page_path does not take path aliases into account.
This is not a problem confined to the group module. User entities also have no entity_ids:
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
itmaybejj commentedComment #4
maskedjellybeanMR opened. It appears we can fix the entity_type, entity_id and page_path just by changing that one bit of code.
However, this should not be merged as is, because it causes duplicate data. Because the entity_id is now correct, the previous row associated with the pid and previous entity_id (0) is not found, and a new row with the correct data is created:
Comment #5
maskedjellybeanYou'll also have to decide whether you want entity_type to be the actual machine name of the entity or a human readable version that is used elsewhere in the code ("Entity: User"). Using the machine name means you can easily load the entity if need be. If there's a need to display "Entity: User" somewhere, can't that be done during render somewhere?
Comment #6
itmaybejj commentedI'm going to have to really, really dig into this to get you some concrete answers and confirm your MR will work across the board; this part of the code is fantastically prone to conflicts with other modules and custom entities that do not quite fully implement various interfaces, so I am always juggling accuracy and completeness with "not crashing in production." I'm going to try to replicate your setup to see if I can trip the null
entity_id; this is the first I've seen that, and I rely onentity_idin a lot of the dashboard code, so it needs to be right.Up until now I have used
route_name+entity_idfor Views reference plugins, and only intended entity_type as a human readable bundle label for the most precise bundle, as a filter for the dashboard. Thus all the various fallbacks, for entities that don't have a traditional bundle.Page path should be pulling whatever the entity itself calls canonical. That may or may not be aliased.
One question to quick confirm before I dig in: do you see all these issues with freshly syncing data in the latest 3.x release with a fresh module install, or is there migrated 2.x data in your tables conflating things?
If I get all this replicated and working, I should be able to merge a version of your fix and layer on an update hook that runs a batch correction for affected rows based on path and route name.
Comment #7
maskedjellybeanThis is migrated data from 2.x where this issue existed in 2.x and continues in 3.x. Actually part of the reason I upgraded to 3.x was to see if it fixed this. Before making any changes I refreshed some pages where the entity_id was missing or incorrect, did some step debugging, checked the database rows and found that the issue was the same as 2.x. So I don't think the issue is really related to migrated data from 2.x.
The problem with the bit of code for group compatibility at least is that the path to group content is /group/{GROUP_ID}/content/{GROUP_CONTENT_ID}. Since the code was grabbing the first section of the path and assuming that was the entity type, it was finding "group" and not "group_content".
I think it's safer to get the entity type from the route name which we already have if the route name starts with "entity." and ends with ".canonical". I've never seen an entity canonical route that didn't follow this pattern: entity.ENTITY_TYPE.canonical
This way we're not looking at the route parameters and trying to predict what they may be for the entity type.
BTW I see this change fixes missing entity_id for users as well (after adding an empty H1 to user.html.twig and viewing a user):
Comment #8
itmaybejj commentedThat makes sense. I'll get Group installed and added to my unit tests in the next release with this so I don't regress in the future.
Comment #11
itmaybejj commentedI have not yet tested my new merge request for this so don't try it in production, but if you are up for helping me with testing, since you have real data and I just have a sandbox, in theory it should:
Comment #13
itmaybejj commentedComment #15
itmaybejj commentedComment #16
itmaybejj commentedOK I tagged 3.0.4 with the fixes.
Contact me if you want a year of CSA for helping test.
Comment #18
maskedjellybeanWow this is more than I ever expected! Thanks!
I'm sorry for disappearing on you. I want to be helpful and test this but I had my head down making group compatible views and we went different directions. TBH it might be for the best because the group setup on my site is a bit custom to say the least, which I'm sorry I didn't factor in when I opened this issue. Due to decisions made years ago without me, we render the related node on the group content route (we only allow a node to relate to one group). With this setup I doubt I can fairly test your solution. What I can say is that I was able to make my group compatible views (which I started by copying the ones provided by editoria11y) accept a group id as contextual argument and I was able to create the necessary relationships without additional code. But this was only after I patched editoria11y so that a group_content page has entity_type "node" in ed11y_page table and entity_id is the node id. I realize that is far too custom a patch to contribute back. I wish we had a normal group setup, because it would be great to contribute my custom module (with a controller, local task and group level permission so group members can access the report at /group/GROUP_ID/reports/editoria11y), but I'm afraid all it would do is cause more confusion.
If I can find a bit more time I'll see if I can apply your solution and see how it plays with this unique setup, but of course that won't be a good test of a normal group setup.
Comment #19
itmaybejj commentedAhh fair enough.
If you want to convert the code in your patch to something less fragile, you may want to look at the new editoria11y_alter_config hook. That lets you read and modify any of the page data from the dot-module file (like entity type and ID) on the fly.
I needed a hook for role-based split config and figured other people might want it too.