🇺🇸United States @bradjones1

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 ✨ Generate JSON schema for content entity types Needs review 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.

I didn't know about PriorityTaggedServiceTrait prior to this, but like how it is purpose-built for this. We just need to implement it. Very straightforward, I think this is RTBC.

Beyond the test that's in the MR? AFAIK this isn't test-able in the UI since you can't create a custom entity type in core via the UI.

A bit of a fat-fingered rebase but I am hoping that this is very close to RTBC still.

I have addressed your comments.

ptmkenny → credited bradjones1 → .

Made a few notes, very minor. I think this is generally RTBC.

As we are no longer going to do Brad Jones' "Drupal core's out-of-the-box JSON data storage", I have no problems with lowering the minimal supported version of SQLite.

This is news to me.

we need to deal with original and also other stuff that's thrown on entities at the moment. And there's no movement at the moment on those issues.

This isn't universally true. See 📌 Define 'original' as property on the entity object Needs work which is getting closer and closer and just needs a little more love.

Re: #8, see #3452756-18: Ensure querying JSON nested values when parent keys are unknown is possible in all supported databases → for some notes on the JSON querying side of this.

Ted name-checked me in #4 and we did talk about this in some depth in Slack, though that conversation didn't get reflected here.

I am not working on XB but appreciate the intersection of JSON data and XB, and the kinds of problems you're trying to solve.

The issue, really, is SQLite. As it has been all along with JSON data storage in core. SQLite is just not on par with the other supported drivers.

Querying for an expression within a JSON document is possible in MySQL/MariaDB and Postgres with JSON_CONTAINS() in one form or another. There is no equivalent for SQLite.

In Slack, I suggested:

if you must support SQLite, extract the part of the document that contains the thing you need to filter and implement the rest of the JSON_CONTAINS type operation in PHP.

And for supported drivers, we can implement a jsonContains() method on the new JsonConditionInterface.

While I haven't reviewed all this issue's history, I can say confidently that any code that builds a JSON condition expression without using the query builder is a fool's errand. There are too many edge cases and this is a great illustration of why we have a DBAL in the first place.

I honestly don't think you're too far from being able to do something like this in XB, though it would require a special-case for SQLite as described. I don't necessarily think that's the end of the world, but not for me to decide.

Re: the conversion burden this might cause, this seems to be the kind of thing that can be aided by a rector rule?

hestenet → credited bradjones1 → .

The idea is that your code will programmatically create the membership, e.g. by using a membership provider tied to a payment processor. https://github.com/drupal-membership/

This module is very much a work-in-progress API for managing memberships that are a reflection of an external state machine/system of record.

Yes.

Thanks for referring this over to me for review.

Merged the PU bot proposal. As there was no other substantive work done here, no other credit.

bradjones1 → made their first commit to this issue’s fork.

bradjones1 → created an issue.

@Akhil Babu, thanks for this! I think this change could be made pretty easily, and your MR gets at the core (only?) real code change that needs to be made. (I think a LogicException might be more appropriate here?)

What this needs, though, is tests that validate the universe of allowed and disallowed patterns. That would also help in documenting in code what is and isn't allowed by Drupal's route builder.

bradjones1 → created an issue.

https://git.drupalcode.org/project/openapi/-/commit/31ce21815e3885812bdc...

I think maybe Dave needs to trigger it? "You are not authorized to trigger this manual job."

Yeah to be clear I'm not throwing cold water on the idea so much as playing devil's advocate. I think the sensible defaults you propose are sensible-enough to also allow people to do a definition long-hand, as it were. You can be very explicit or just set the tag and let the builder do the rest.

bradjones1 → created an issue.

bradjones1 → created an issue.

This module is not yet stable. Thanks for pointing this out. Please do not use 3.x in production (yet).

I think ##7 is an interesting compromise, however after spending some time working on DB backend-overrides (which are kinda broken in HEAD) that also depend on a bit of heavy definition-adjustment in compiler passes, I do have some concern over how much magic we do behind the scenes. The YAML definitions themselves are not complete, as it were, and you have to "know" it's being fleshed out elsewhere. That's not to say we shouldn't do this, but it would be the most significant case of Drupal-y DI autoconfiguration we have I think.

Looks like all tests are passing now, next step is to incorporate some of the changes from the alternative MR.

The failure on SQLite is 🐛 GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite Needs review . Seems like there is some sort of regression with PgSQL though... playing whack-a-mole. This issue has turned into fixing what appears to be a very-not-stable backend-override API more than the original goal. Gotta fix all that though, at this point.

Re: #75 I recall asking about this... somewhere... (can't find it in this thread, maybe Slack) and the answer is, not really.

I _suppose_ you could do some container-building magic and configure the factory and arguments there from the tag (or vice-versa), however I think that's too auto-magical. In theory you need not use the cache factory, right, so we don't want to hard-wire that.

I think the issue is one of context - the cache factory doesn't have access to the tags to pull a default name from there.

Rebasing is preferential IMO to merging in from upstream, it helps keep the changes in the MR clearer. But, whatever, certainly not a hill to die on.

I think the performance test query list is more about order of operations and may even reveal a lack of test coverage - I'm running into that a lot, lately. Not sure it's really a failure vs. an opportunity to update the test.

I'm ambivalent on a flag that indicates the specific nature of the read-committed implementation. It seems like it's mostly only really useful for tests, but perhaps a public method for indicating this behavior would help some power-user developers. I'm tempted to not add to the API surface of the connection for something that we can address in tests - it's only really a MySQL vs. Postgres difference. But I don't care much either way, there are bigger issues uncovered here.

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

I don't think we really have any alternative. I would suggest couching it as "SQLite diverging from MySQL/MariaDB/Postgres," and not the other way around. As explored in detail over at ✨ Add "json" as core data type to Schema and Database API Needs work , SQLite is divergent from the other, non-file based drivers in a number of ways and I imagine we will continue to root out these differences as Drupal becomes more advanced and modern under the hood. I think there is a loose but growing consensus that running Drupal on SQLite in production will mean accepting these limitations or trade-offs. If we insist on 100% parity, it means that we sacrifice innovation for a small minority of implementations. I don't think we should do so.

Recapping my debugging this morning.

I think backend-overriding queue.database has been broken since at least 📌 Convert QueueFactory to use a service locator Fixed introduced a service locator.

Here's what happens:

• BackendCompilerPass adds an alias for the overridden service's ID pointing at the override service.
• The service locator is looking for services tagged with queue_factory, which now results in an empty ServiceLocator passed to the queue service.
• CoreServiceProvider does tag instances of QueueFactoryInterface with queue_factory, but that doesn't appear to extend to services defined by modules, including sqlite. I think that it probably should but I'm not sure why it isn't working. Probably an issue with order of operations in the service providers and passes.
• Manually tagging sqlite.queue.database, the backend-override service, as a queue_factory results in it showing up in the service locator instance passed to the queue service, but it's named sqlite.queue.database, not queue.database, which is the hard-coded default that is requested. Service locators aren't aware of aliases.

Even if the override service got the tag automatically, that doesn't change the fact that queue is requesting queue.database as a default, which is an alias. I think the answer is in how BackendCompilerPass registers the overrides. In cases where the service is requested directly from the "main" container, this all works fine. But laziness causes this go to awry.

I'll see about opening a separate issue to track this, but it is a hard blocker for the sqlite carve-out we need here.

SQLite tests failing in a new way but much closer. Will pick up tomorrow.

Updated this in the related MR - https://git.drupalcode.org/project/drupal/-/merge_requests/8273/diffs#25...

The issue isn't so much that the lazy services lose their tags, it's that backend_overridable in particular is sensitive to the service ID, and the tags on proxied services go with the new 'drupal.proxy_original_service.' . $service_id service, not the replacement service which would match the overridden service e.g. in a database module. So this is why the issue is unique to backend_overridable, but not other tagged services like service collectors.

There's a fix for this included in this MR https://git.drupalcode.org/project/drupal/-/merge_requests/8273/diffs#25... on 🐛 Race conditions with lock/cache using non-database storage - add a non-transactional database connection Needs work if we might just be able to fix it there.

So I chased my tail a bit on sqlite-specific stuff, namely that I forgot to put the dblog override in a service provider because otherwise it registers itself as a logger when dblog module isn't loaded, and that breaks things.

I also discovered that you can't currently do a backend-override on a lazy service - see 🐛 Lazy services (backed by proxy classes) lose their tags, e.g. `backend_overridable` Active - I'm including a fix in this issue but if it needs to be broken out into a separate one... then at least there's a separate issue. Oy vey.