Validate entity bundle in asset/log/etc page_type display logic

Thanks for opening this! I think I found where this is happening.

I first checked to make sure that the page_type displays for assets and logs had the contextual filter configured to "Show page not found" if the filter does not validate. This is configured correctly, which makes me think the error is happening somewhere else.

Looking at farm_ui_views it seems that we assume the "bundle" we pull from the path argument is always a valid bundle, but we never actually check this. I think the error happens when trying to load bundle field definitions (I tracked this is a bit in my debugger):

Bundle id pulled in here: https://github.com/farmOS/farmOS/blob/96cbf7d4d5f7c889dd31062a8f8507632e...

And bundle fields loaded here: https://github.com/farmOS/farmOS/blob/96cbf7d4d5f7c889dd31062a8f8507632e...

I think we just need some logic inside farm_ui_views_add_bundle_handlers to ensure the bundle is correct.

Although I'm curious if we could change the $bundle argument to be an instance of the EntityTypeInterface itself. That way the caller of the method needs to make sure they have a correct bundle. Not sure which would be preferred here.

Good sleuthing @symbioquine and @paul121!

I think we just need some logic inside farm_ui_views_add_bundle_handlers to ensure the bundle is correct.

What do you think about doing this one level up, in farm_ui_views_views_pre_view() instead? This function calls farm_ui_views_add_bundle_handlers() twice, but we only need to validate the bundle once.

I opened a MR with a proposed fix. Let me know what you think.

This can probably be a "minor" bug report, because it requires intentionally going to a non-existent path.

Ah dang, the issue title got changed again (why does that happen??) - I'll take the opportunity to make it a little more specific. :-)

This change works and solves the initial problem, but a 500 error can still be created if an incorrect bundle is supplied when requesting the "by type" geojson endpoint.

What do you think about doing this one level up, in farm_ui_views_views_pre_view() instead? This function calls farm_ui_views_add_bundle_handlers() twice, but we only need to validate the bundle once.

It would be safer to include it within farm_ui_views_add_bundle_handlers() since that is where the problematic function call actually exists. If this method is used anywhere else then they would not need to perform the safety check themselves. Not sure if this function is meant to be used by contrib (although I think it would be useful and work fine in other contexts), but perhaps we should mark it as "internal" regardless?

*And actually, as a matter of fact, we do use this function elsewhere in farm_ui_map.views_execution (it is quite useful!): https://github.com/farmOS/farmOS/blob/ef14113282a31fc6d36e4c735d717fa520...

But because (with the proposed change) the view is never rendered for invalid bundles and thus the map doesn't get attached, this code in farm_ui_map.views_execution isn't really a problem.... unless the asset geojson endpoint is manually queried for an unknown bundle... but still.... maybe we should give this some thought.

This makes me lean towards my latter idea in #2 of changing the argument to an instance of EntityTypeInterface. This way the caller is responsible for checking a valid bundle. I think this would be the most "correct" approach if we were to advertise the function as available and supported to contrib modules.

Ah good points @paul121 - I overlooked those cases.

changing the argument to an instance of EntityTypeInterface. This way the caller is responsible for checking a valid bundle.

Makes sense to me!

(Oops that status change was a mistake.)

Something that just comes to mind... perhaps a simpler way to solve this would be to modify the route definition so that the bundle path parameter is cast as an entity type. I believe this would trigger a 404 error at the routing layer if the entity bundle does not exist.

I've done this in a view of plans in another project so that an action link is only provided for this bundle: https://git.drupalcode.org/project/farm_project_plan/-/blob/1.x/src/Rout...

But making this change would effect some of our existing code that expects the bundle param to be an ID, not an entity object, so there are some considerations. Maybe we could adopt this for all farmOS views? There is an open issue for this in core [#2528166}

Forgot about this issue... found it while cleaning up the issue queue and opened a quick PR with tests: https://github.com/farmOS/farmOS/pull/1048