Re: #9 - yeah, I tinkered with that a bit as well (simply removing it seemed to yield the promised crash) but I think these are separate but similar issues?
Test failures have me worried that perhaps some code is depending on "broken" functionality? Hmm.
Filing this against 3.0.x because I think that's the correct thing to do, but this should hopefully also get a backport to 2.x. Blocks 📌 Undo temporary workaround on order item/order save process Active on Recurring which allows us to remove an ugly work-around and fix some test coverage.
Test coverage was incorrect to begin with, I think... and this is postponed on 📌 Saving an order with unsaved order items results in tough-to-debug data inconsistency Active in Commerce core which allows us to back out the work-around in recurring.
Re: #6:
Can we just use
\Drupal\Core\StreamWrapper\StreamWrapperInterface::realpath()?
No, because some stream wrappers have no "real path" that's applicable here; the "directory path" is being used (abused?) to generate a route that is _both_ registered as valid in Drupal but also on disk, with the expectation that the file on disk will "win" when the server receives the next request. Clever, but in this case a little too much so.
Narrowed up, hopefully this is easier to get in without the distraction of how to improve the (kinda currently broken/invalid) cache-control headers.
Re: #12 - that's interesting history, thanks.
I've opened
🐛
Asset cache-control headers are incorrect
Active
to break out the issue of the cache-control header from the stream wrapper question, so we can perhaps at least unblock this restriction on asset storage from the question of the headers.
Regarding treating the response "from Drupal" differently from subsequent requests for the same file served off disk:
I've seen situations in the past (maybe Drupal 7 era) where you end up with different versions of supposedly the same thing in different CDN endpoints and it was not enjoyable. Even something like different max age/expires headers could be annoying.
Certainly this could be an issue, but we also need to consider the increasingly-common cloud native use case. As it stands, there's no out-of-the-box option for here site owners to deploy Drupal in a runtime with no publicly-accessible locally-persisted storage and enjoy the benefits of cache-friendly headers.
The default use case is certainly the typical model of "public files on disk," and that should be what Drupal expects out of the box. What we should do, then, is at least support sending cache-control headers that are CDN-friendly for caching the "PHP response" without needing to patch core.
This is expert-mode stuff so I don't think it would be out of school to put this behind a container parameter? (I really don't like introducing new "Settings settings" in 2025/Drupal 11, but I feel as though I may be tilting at a windmill there.) That would at least provide a hook-point for customizing this and serving both use cases?
Copying over comment from @catch prior to this being split off:
@bradjones1 the main reason to prevent sending cacheable headers there is to avoid, for example, different CDN endpoints from serving the PHP version of the response vs. the file as it's served from the web server direct from disk. Even if the content can be guaranteed to be exactly the same, the headers would be different.
There are two reasons for this apart from consistency:
I've seen situations in the past (maybe Drupal 7 era) where you end up with different versions of supposedly the same thing in different CDN endpoints and it was not enjoyable. Even something like different max age/expires headers could be annoying.
Also some hosts, the only example I know of is Pantheon but there may be others, bill differently for requests served from PHP vs static assets from disk, even when those requests are cached in the CDN. I don't understand why they make that distinction but it's how I interpret this documentation:
https://docs.pantheon.io/guides/account-mgmt/traffic#pages-served
I will elaborate on my thoughts on this in a separate comment, but I have poked @greg.1.anderson at Pantheon for some clarification on this front, as it does seem a little curious that different cache entries would be billed differently on a CDN based on the origin.
bradjones1 → created an issue.
Upping priority and changing component.
Ran into this in recurring, namely 📌 Undo temporary workaround on order item/order save process Active . In that case, recurring does a lot of "programmatic" order building that is not spread over multiple bootstraps/client page views, and so the $order object that you are building, adding (unsaved) order items to, rebulding, recalculating, and saving results in an incorrect order total.
I believe the real issue here is that Drupal's core Entity API has no problem saving a yet-unsaved entity reference when the hosting entity is saved. For commerce, since we maintain a lot of bidirectional entity references, this means EntityReferenceItem::hasNewEntity() will return false when called during the order save... because the order item was just previously saved by EntityReferenceItem::preSave().
I did a little experimenting with explicitly setting the is-new value with ::enforceIsNew() but I started getting errors about a duplicate insert into the database and backed off. Could still be a path forward for us but perhaps it's a little too cute.
The big issue here is DX. You think the thing will work, because it really should and does in this way around the rest of Drupal... but Commerce is really doing wild stuff behind the scenes and can't take it. It doesn't fail, though, leading you to know somehow that you need to always save an order item before it goes on an order.
Since 3.x is open now this is an opportunity for BC breaking changes (if we can't stomach throwing these exceptions in 2.x) but I think perhaps adding this in 2.x would be a good idea to uncover errors that are very likely in the wild, today, e.g. wrong order totals.
I need to check out for the weekend but I think this sums up well the underlying issue of an unsaved order item being saved during order save.
It might be that this is in fact just unsupported in commerce. If that's so, we should detect this condition early and throw an exception, because otherwise this 1) requires developers to magically know they can't rely on functionality that otherwise "just works" in Entity API without breaking those contracts and 2) results in broken order data (namely, the total) in a rather undeterministic manner.
This is covered in 📌 Saving an order with unsaved order items results in tough-to-debug data inconsistency Active so I'll put more discussion there. For recurring, the question is whether we accept this bug is really in Commerce Order and work around it here? Probably. That would mean keeping the workaround in place, updating it with a reference here, and then acting accordingly in the recurring ecosystem.
bradjones1 → created an issue.
Very old issue.
Providing an explicit link to 🐛 AssetRoutes::routes() assumes stream wrapper implements ::getDirectoryPath() Active which illustrates this well.
This is postponed on core supporting this in 🐛 AssetRoutes::routes() assumes stream wrapper implements ::getDirectoryPath() Active .
Thank you all for chiming in on this. I hope my responses in the MR thread addresses much of the questions around the why and what.
I would like to summarize the reasons for supporting non-local stream wrappers, however. Catch's comment sums up the issue well:
The code comments appear to indicate they the alternative stream wrapper is never used to directly serve a file. In which case why change the stream wrapper at all - could override the controller and use a cache backend to store the file contents instead.
There's two main issues at play here. One is the time to generate the asset, which we can all agree is significant and we want to avoid doing so more than once. Second is the question of how and where the client requests the generated file on subsequent requests.
The first concern is addressed out of the box no matter the local/non-local status of the stream wrapper. The asset controller doesn't find a matching file (loaded with assets:// stream wrapper, works no matter the backing storage), generates it, saves it and sends the result to the client.
We are addressing here the second issue. It is the stream wrapper's job to yield an external URL for the file - see \Drupal\Core\StreamWrapper\StreamWrapperInterface::getExternalUrl(). For local stream wrappers, this very conveniently matches the paths from AssetRoutes::routes(). For others, the stream wrapper must special-case `assets://` and point the external URL at the asset controllers. (I've linked an example from an MR against Flysystem, above.)
Subsequent requests for an already-generated asset would be piped, but it still has the overhead of bootstrapping Drupal. This is where the site owner's responsibility lies to have that external URL behind some sort of caching reverse proxy.
I'm proposing the fix we have here because it 1) doesn't change anything about the default case of using local storage and 2) allows expert site owners to utilize the core asset generation API but in a way that gives you maximum flexibility about the backing storage. For instance, a Drupal application that has many web containers running in a serverless environment wouldn't need to each have its own memory cache copy of the asset.
Major as this blocks alternative implementations for assets:// - "Render one feature unusable with no workaround"
This is 🐛 AssetRoutes::routes() assumes stream wrapper implements ::getDirectoryPath() Active I think.
bradjones1 → created an issue.
bradjones1 → created an issue.
This likely needs a CI run but RTBC'ing this as I've implemented on a site I'm building and looks good to me.
I'm going to be bold and close this. I am implementing subscriptions with Stripe and anonymous checkout on the Drupal side and out of the box this appears to work. A lot changes in payments in 5+ years. Can re-open if necessary.
Ran into this today but in commerce_checkout. Basically if you have deleted the default product variation type prior to enabling commerce_checkout (because you're a brash power-user like me and go wild in the UI when you first install stuff) then you'll get
Configuration objects provided by <em class="placeholder">commerce_checkout</em> have unmet dependencies: <em class="placeholder">core.entity_view_display.commerce_product_variation.default.summary (commerce_product.commerc
e_product_variation_type.default)</em>which is basically the same underlying issue.
This may or may not still be an issue and relates to a very early development version of this code/the core patch.
Re: ##8 - thanks Wim for your very kind words.
The change record doesn't mention it, but the commit does change the JSON:API version: JsonApiSpec::SUPPORTED_SPECIFICATION_VERSION = '1.1' is now a fact. So 🌱 [Meta] JSON:API 1.1 spec compliance/support Active is partially done? Completely? See #3305324-18: [Meta] JSON:API 1.1 spec compliance/support → . It seems that meta indicates there's a lot more to be done to be 1.1-compliant, so it's a bit surprising to see that constant being changed. I think either an updated change record that clarifies it, or an additional change record would be appropriate?
I bumped the version in this MR for a few reasons. One, our testing and validation features (e.g., that validates responses when assertions are enabled) depend on a schema for JSON:API itself, which is not exactly stable but the upstream maintenance of JSON:API overall is glacially slow. The schema file we had for 1.0 was draft and not that valid and so the updated 1.1 draft is much closer to truly representing the spec's requirements.
After reviewing the linked issue I would say that we very nearly support 1.1 at a basic level, though we "should" also finish 📌 Spec Compliance: JSON API's profile/extention (Fancy Filters, Drupal sorting, Drupal pagination, relationship arity) needs to be explicitly communicated Active to convey our profile.
TL;DR, it's time to bump it because we were far ahead of 1.0 with implementing features that would later go into 1.1. We don't strictly validate, but it's much closer to the current spec than we've ever been.
This seems to statically define JSON schemas, which is not good enough: many field types have settings that impact what the correct JSON schema would be. IOW: the JSON schemas have to be dynamically defined for them to be precise. For example: min/max for integers and numbers, a limited set of valid values (enum in JSON schema), many strings have a particular format (https://json-schema.org/understanding-json-schema/reference/string#built...), and so on. Are there plans/discussions for how to support those more precise JSON schema descriptions? Perhaps this needs a follow-up?
Schemas _may_ be defined statically, but they need not be. In addition, JSON Schema generation has no caching layer (the assumption being that the caller is responsible for this, e.g. OpenAPI module, and invalidation is based on entity definitions or whatever) so the entire schema for the normalization you are introspecting is generated on-demand. It is true that included in the merged MR was a trait and attributes that help provide static schemas... but that's not a requirement. This allowed us to add a lot of default schemas for primitive types and make it easy to define them this way, but you can also return any valid schema generated any way you like. So I think a follow-up for option module to provide a ListStringItemNormalizer with its own dynamic logic would be amazing. But it requires no change to the underlying API. Perhaps we could introduce some additional traits or whatever to assist with code re-use, but yeah.
8273 is the active/main MR for review. Updated per feedback, rebased and back to NR.
Drive-by idea: Is the autowiring logic in Symfony able to be extrapolated/re-used here to basically allow for autowiring similar to services? It's the same thing... except these aren't first-class services. Or am I way off?
To clarify what I mean in the prior comment - I use assert() in particular to check the order of services when I have multiple layers of decoration. So niche case but it was unclear the way I phrased it.
In my practice, I definitely find myself wanting something like this but also my IDE knows what to expect once I put in an assert() on the next line. This has the benefit, also, of validating that I am getting the service I expect during tests and local development. So personally this is in the lower-priority bucket as there are other Drupalisms in the service container I'd like to tackle first... BUT if this did all the things out of the box as we'd expect, then that's great. But I agree if the edge cases are just as tricky as the status quo, there's no benefit.
Re: #13 - I can at least answer the question of, what service do you get? You get the decorated service. This is as it works now, at least if the service's name is the same as its original class.
The bigger issue is that what you really want is a response containing a predictable interface. That's harder to typehint. If we could solve for that edge case (or it can be explained a bit better what you get in various circumstances, e.g. a truth table) and it works with IDE completion, this would be amazing.
Thanks, Wim!
I will reply to your specific points soon, but why is this now marked as to-be-ported? The reply above indicates this can't go into 10.x, so I think there's nothing to backport?
(To be fair, my main project in production is "still" D10 and this applies relatively cleanly, but I also understand if it can't go officially into D10 because of policy.) I would love for this to be supported in D10 as that would personally benefit me and make the new majors of OpenAPI/OpenAPI JSON:API D10 compatible but appreciate if that's just too much.
Personally, I'd like to drop entirely the habit of setting random data on entity objects.
I'd agree with this. There are other options (e.g., the tempstore) that work for most of the use cases described.
I'll drop in here and suggest that some variation of JSON storage for this could be a nice way to interact with this and avoid some of the weirdness in the user data model, which is truly prehistoric Drupal.
Addressed MR feedback and made a few changes. Tests still green, back to NR.
Is this eligible to go into D10 LTS?
Back to NR.
This is likely due to ✨ Allow specifying `meta` data on JSON:API objects Needs work going in which is a good thing. Needs a rebase then should be back to RTBC.
bradjones1 → created an issue.
bradjones1 → created an issue.
Related/potential duplicate that addresses this more broadly: 🐛 Not all URIs are URLs, but UriWidget won't accept non-URL URIs Needs work
I'm not sure if there is a policy that speaks to this but a CR would be good as this is a pretty important change to the base test setup and is not obvious.
Back to NW as it seems the broader question is whether this should happen at all.
Re: #10, I think this is a misunderstanding of how doxygen works. They are commented out by virtue of being doxygen.
Re: #11:
Further this would add thousands of Attributes which do get checked.
I think this is a feature, not a bug?
My IDE already will show a message if you're not matching the interface.
Sure, but as noted above - I do not believe it will necessarily catch the situation where you intend to override a method that is no longer there. That's the real benefit of Override.
See 📌 Remove damiankloip as subsystem maintainer Active for follow-up per #8.
bradjones1 → created an issue.
Fine by me. The code change is negligible, I can update the MR today.
That linked code snippet is Exhibit A on why there needs to be an API-level fix for this issue. So many caches/states to reset. And I'm sure they were all hard-fought to discover.
Thought - could it be that at least in some cases this value was set to a PECL parser, which would be incompatible with 📌 Drop PECL YAML library support in favor of only Symfony YAML Fixed ? We would perhaps need a test/conditional to validate the provided class is a Symfony parser? That is the part of the BC layer here that we really can't provide without backing out the whole of the change that got us here, in the first place.
Hacked a bit more on this tonight and am more happy with where this is going. Kind of a compromise between the two MRs.
Per #95:
In the alternate MR, made the interface empty. So we can typehint with the interface where we can accept both Connection and NonTransactionalConnection, or the more specific class where needed.
I toyed with this but eventually landed on a fuller interface. Reason being, the non-transactional connection can "do" most everything the wrapped/transactional connection can, so let's make it a first-class citizen. I think it's also good to move toward an interface for Connection, instead of treating the abstract class as a makeshift interface. I am not wed to this but the interface does help with static analysis way more than using __call().
Also, tried a different approach to mark a db driver allowing or not concurrent non-transactional connections, which does not involve services.
I pulled the flag into my MR and the new interface. I kept the service override, however, since SQLite is still the outlier here and all the services in question are backend-overridable. This is what that model is for, so let's use it instead of hiding the logic.
Finally - diverging MySql from SQLite here is starting to scare me a bit, we would end up everywhere in very different db state results depending on whether the db supports or not the concurrent non-transactional connection...
As discussed a bit above... I'm not sure there's really any way around this. Yes, you will get some different results with SQLite but that's no different than now, and in the vast majority of production use cases this will result in more deterministic and predictable behavior, not less. I think that's a win.
OK, so the MR is updated for 10.5.x, it keeps the deprecation but allows this to honor BC.
I went back and forth on whether to propose "keeping" this functionality in D11, as some have noted here and elsewhere there are legitimate use cases for swapping in your own Yaml parser. I am only doing so to get functionality that will come in 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work ... BUT this shouldn't have broken D10 sites to begin with and we need to fix that.
I've rebased against 11.x, if it doesn't apply to a specific tag it would be because of a change in HEAD.
larowlan → credited bradjones1 → .
If you're using the PECL parser AND the extensions aren't supported, that's on you IMO.
I am only extending the Symfony parser to allow for functionality that is forthcoming in 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work but I'd rather just keep my predictable code path and upgrade with 11.x than add another patch to core on my project. I think there's a case to be made also to maintain some sort of hook point for a custom parser for the reason that core might not always have the specific parser config (e.g., flags) you want.
It seems to me that this was removed, not deprecated, and this is a BC break in violation of our usual way of doing things. Seems to me that a deprecation should mean the existing functionality works in 10.x as much as possible. At the very least the deprecation should be re-cast as a breaking-change removal of functionality and not a deprecation, which it isn't right now.
In short, no. I have no paid sponsorship to work on this module, so it's entirely my donated time while working on my in-house product. Also, this is blocked on ✨ Introduce "schematic" normalizers Active being committed to core. If you can lend any help there, that's the main obstacle.
I appreciate the pointers to these issues re: the Symfony Yaml parser, however I don't see how this is pertinent to the issue of not being able to provide your own overridden config. This isn't a function of the PECL parser.
Are you saying that a "proper" deprecation dance isn't possible here?
bradjones1 → created an issue.
The relevant code was removed in 📌 Drop PECL YAML library support in favor of only Symfony YAML Fixed
Rebasing.
Can you elaborate on why empty field item lists need special handling, i seem to be missing or not fully grasping something on that front?
Empty lists are empty for a reason. However, if we can't attach cacheability data to that emptiness, then when the empty condition changes (e.g., due to something that does not otherwise cache-invalidate the entity) then you'll never see the new, populated data.
Re: #76 thanks for the kind words.
I'm not really sure how to address the questions about data migration. AFAIK there is no statement of policy or procedure for an official path to migrate Drupal sites from one database type to another. Given there is a lot of compatibility (but not perfect...) between SQL syntax, it's likely that basic migrations from one db type to another will largely work, but this is an area where support and syntax does differ.
I have asked the question in the course of developing this feature - do we need to support moving from one DB to another? And the answer has so far been mostly silence. I don't think this is a common workflow. That said, your example of in-browser Starshot demo being exported from, say, SQLite to MySQL or MariaDB is a concrete example. I wonder if @mglaman has thoughts on that.
All this being said, I don't think any of this is a particular reason to (further...) delay getting this into core. Supporting something that has never been explicitly supported or inventing the equivalent of a commit gate over this would not be a good move. (Not saying you're suggesting this.)
Well that was a fun afternoon of chasing my tail on a workspaces integration test only to find it was due to an out of sync static cache between the test execution environment and the test web server. Oy vey. Makes me yearn for #3324241-8: Provide DIC-friendly, concurrency-safe alternative(s) to `drupal_static()` → or similar fixes that will allow us to avoid this kind of WTF in the future. Not to mention better support for async runtimes. End rant.
There seem to be an awful lot of "random" test failures lately in functional tests, but I can't recreate any of them locally. I think this is ready for fresh review.
Updating layout builder to use the new storage mapping was trivially easy. There will need to be a migration of existing data, though, and I honestly don't have personal interest in writing migration tests right now. Hopefully someone with more skill and specific knowledge of that module can help out over at 📌 Do not store sections as PHP objects Active . The BC layer introduced in the commit above basically means that these sections will be updated as they are touched and saved, but we will need a migration to capture them all if we indeed deprecate and remove support for PHP object serialization in the database.
Speaking of, I have stubbed out a CR for deprecating PHP object serialization → , linked to this issue. This is in response to MR feedback asking why we couldn't add ['allowed_classes' => FALSE] to the unserialize call. There's a reason why it's not there currently - it would preclude behavior like seen in layout builder. In fact, there's even a test (also now deprecated) for this behavior. If we need to keep it, that would be fine and simplify things here... but it's probably the right thing to do.
bradjones1 → created an issue.
Adding an explicit pointer here to
📌
Allow field types to control how properties are mapped to and from storage
Needs work
which would potentially provide a path to not needing \unserialize() and deprecating serialization of PHP objects.
So latest test reveals that if we restrict unserialize() in SqlContentEntityStorage, layout builder demonstrates they are depending on this functionality - but they're the only ones doing so.
I imagine there could also be contrib field types that are depending on this unrestricted unserialize(). That should probably be deprecated officially... but as it stands I think this means that layout builder needs to implement this change at the same time?
Moving LB internals to use JSON or this new interface would unblock at least two LB issues postponed on the unserialize() aspect.
This is so close but a lot of this code is also in 📌 Allow field types to control how properties are mapped to and from storage Needs work which is getting focused review, so let's get that sorted out.
Updating tags. Marking this as a contrib blocker because new versions of OpenAPI and OpenAPI JSON:API depend on this.
Putting this back to RTBC as it only needed a phpstan-related update relating to improved analysis as this awaits commit.
Thanks... I have read and accept them.
bradjones1 → created an issue.
It is possible this is due to a missing or broken comment type. See the just added related issue.
I'm going to close this as won't fix/duplicate as the underlying issue appears to be either misconfiguration or an orphaned comment. Both of which can be handled in their own right.
bradjones1 → changed the visibility of the branch 3479887-random-test-failure to hidden.
All green on pgsql.
https://git.drupalcode.org/issue/drupal-3479887/-/pipelines/321059/
This is consistently reproducible locally... in fact it's less random and much more like, totally broken.
The test is set up wrong. Fixed test MR proposed.
Just ran into this as well. https://git.drupalcode.org/issue/drupal-3343634/-/pipelines/320105/test_...
I'm a little confused about your note in #92 about fibers - the request handling is still wrapped in a Fiber, but this is only to provide some back-up functionality if there is a suspended fiber in the code path? You mentioned removing Fiber-related code so just trying to figure out where this landed.