Media::create() should fail gracefully if called with an invalid bundle

I tested on Drupal 10.1.x, standard install using the steps in the Issue Summary. I was able to reproduce the error.

back to need work for tests.

I'm aware that the previous consensus was to only fix this for the Media entity type because of the perception that it would be too much work for all entities. But I really wanted to see for myself what would happen.

dcam → changed the visibility of the branch 3174255-docreate-invalid-bundle to hidden.

I added a test for the Exception and created the change record → .

I'm not sure why this is implemented as a media specific solution, if anything it should probably be done on content entity storage?

I'm not sure why this is implemented as a media specific solution, if anything it should probably be done on content entity storage?

I had the same question! So I opened MR 11949 to see what would happen if this were done in ContentEntityStorageBase::doCreate(). I meant to document my findings here, but forgot.

What I found was that we take advantage of the fact that the bundle isn't validated in tests everywhere. In fact, we do it so much that it's practically a feature. I'd argue that the idea should move forward, but there's a lot to consider. Simply throwing the exception in ContentEntityStorageBase would be so disruptive that it would likely require a meta issue to manage all the changes across Core. Then we have to consider that if Core's doing it, then contrib probably is too. Again, the potential for disruption is so high that it could probably only be done in a major release because we'd be breaking backward compatibility and modules everywhere might suddenly see failures in their own tests.

That said, maybe we figure out a way to bypass the exception for tests. I'm thinking of something like the flag that skips config schema checking. That may not be a good analogy; it's just an idea. It could be turned on by default for all tests. Or maybe we specifically turn on the flag for tests that are affected by the exception, but then contrib would have the same burden due to BC breaks. It would have to wait for a major release.

After analyzing the impact of changing ContentEntityStorageBase::doCreate() I decided that I agreed with @codersukanta's assessment that the scope of this issue should be limited to MediaStorage since unlike other Core entities it will throw the fatal error from an invalid bundle. How often does that really happen? Probably not often, which I imagine is why this issue's priority is minor. So if a Core manager decided "This issue needs to be refocused for all entities," then that's probably OK. But I don't want to make that call.

Please analyze the other MR and the impact it had on Core tests, draw your own conclusions, and tell me if I'm mistaken or not.

I've updated the IS with this information.

@berdir:

See Comments #13 and #20. Also #35: @dcam tried to do it for the base class and ran into trouble.

@dcam, do you want to try again? If it ends up being a lot harder, then (for the reasons in #35) I suggest that we commit the current MR and open a follow-up issue.

@benjifisher looks like we cross-posted. See my comment in #41.

I'm pretty certain that nodes with invalid bundles also cause a fatal error when viewing due to template_preprocess_node().

But yes, I'm not surprised that a lot of tests are ignoring that at the moment. There are also some weird things like menu_link_content which defines a bundle but does not actually have one.

Can see both sides, that it's more relevant for media, but also it being minor meaning that it's not so important to push a media specific solution through.

The workarounds in the test do seem a bit strange. They are inconsistent, one uses $this->container, the other does not. also sounds like both cases actually do define them, just the entity bundle cache is still. Which is either a problem with mix of web requests and test code, or there's a deeper issue with a static cache not being properly reset, or an issue with multiple containers and DI (that's why we should IMHO discourage and even deprecate $this->container)