🇺🇸United States @bradjones1

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 📌 Add a method to access the original property 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.

bradjones1 → created an issue.

Thanks for the pointer on SQLite - that's unfortunate. As with a few other issues I've been working on lately, it seems this is another area where SQLite isn't really an equally-qualified production DB for Drupal if you're using features that are well-supported in other relational DBs. The test failures re: a locked DB certainly prove out the failure mode you're describing.

The only real workaround on this is to just keep the current behavior with SQLite, that is, don't use the new non-transactional connection. That would involve overriding these services for SQLite (which is already supported in all but one of the services involved - that being queue). I don't think this is a blocker, in so far as we can't not fix the related bugs just because SQLite can't hang.

At first glance, the PGSQL failure seems a little stranger. This is due to a difference in how MySQL and PgSQL handle transaction isolation. Basically, MySQL gives each transaction its own snapshot for reads and writes, while PgSQL allows select queries inside a transaction to see changes committed since the transaction opened. I updated the test to reflect this... it doesn't actually matter, but the test was written too strictly to pass for both MySQL and PgSQL.

Re: the interface question, I'm open to that change, but haven't incorporated it yet into this MR. I think it's better to either have a complete interface or an empty one. That said, I'm not sure typehinting both the non-transactional and transactional connections with the same interface gets us much, because you likely want to use the non-transactional connection sparingly. I'm concerned about situations where you'd get a non-transactional connection passed where it's not appropriate. At least currently we explicitly have to state that it's acceptable.

Added a comment on the alternative MR. My feeling is that we should add the non-transactional class now, which solves a real bug (or two) and move adding a connection interface to a new issue. I certainly did consider the question of an interface for handling this, when pursuing my solution, but it would be a non-trivial deprecation dance to get it done overall. And as you have found, there are wrinkles when considering multiple drivers/db types.

ptmkenny → credited bradjones1 → .

That is so strange, I think something weird happened with changing the merge target. I thought it was strange I didn't need to do a rebase --onto... and clearly that would be necessary. I've fixed this. Thanks for catching it.

Added a basic CR. Tests green now.

Needed a rebase and now GitLab CI is running.

MR target changed.

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

OpenAPI for JSON:API is getting a complete rewrite so I think this may no longer be necessary.

I'd add that the README seems to indicate this was scaffolded from a template or code-generation library. Which is fine, but it also goes to the point already made, that there's not much here to go on that helps demonstrate a minimum level of maintainer competence.

The security opt-in process isn't perfect, and I've been frustrated with it as well in the past, but I would say that you need to avoid things like this that don't help to instill confidence during review.

https://git.drupalcode.org/project/cookie_compliance/-/blob/1.0.x/README...

Related note, so close to having a proper JSON:API 1.0 and 1.1 schema available on jsonapi.org but it's blocked on infra: https://github.com/json-api/json-api/issues/1749

That doesn't mean we have to block this issue on that, and now that it's in a const it's easy enough to update.

bradjones1 → created an issue.

This exact change was made in 🐛 State constraint is not validated on new entities Fixed . Closing this as a duplicate.

I also proposed this over at 🐛 Support validation on new entity/validation not performed on new entities Needs work but it was not linked as a duplicate. Adding a link here for history's sake.

A patch can't be RTBC. Back to NW.

OK, I updated this to handle a similar issue with sample payment entity creation. Ended up adding a mapper to the payment gateway plugin manager to find a gateway that supports a given payment type/payment method type pair. Which is a nice little DX improvement.

Answering my own question, methods and payment types are restricted by the gateway in use. So creating a sample value should draw on that context, if available. Rubber-ducking: https://drupal.slack.com/archives/C1TLCCF9B/p1719209765390819

Working on this further, I believe commerce_payment has the same issue as it also references a payment method. One wrinkle there, however, is that it doesn't appear there's any code path to restrict the payment method type that can be placed on a payment? Do we need a way for payment types to convey what types of payment methods may be used, so we can accurately create a sample value?

bradjones1 → created an issue.

Re: #7, @jsacksick and I chatted a bit via DM. That would also be a potential way to address this, though I am curious about the very Commerce-y angle of the fact that many payment methods are "owned" by the anonymous user, and so filtering by the current user (who could likely be anonymous?) would need to be addressed. I suppose we could further special-case that, but that would perhaps create a hard-to-debug condition for people legitimately trying to do the thing we are special-casing.

Ah good catch, I need to add a conditional in there. This was just my quick-and-dirty from my local project.

Depends on the core issue so tests understandably will fail.

Adding similar issues from Commerce

bradjones1 → created an issue.

bradjones1 → created an issue.

For the dense among us, ISP is https://en.wikipedia.org/wiki/Interface_segregation_principle

Tagging needs IS update for remaining tasks. I am investigating this issue on behalf of a client and would potentially be helping move this along.

Rebased and it looks like there are just a few errors relating to schema validation against the current test coverage.

https://git.drupalcode.org/issue/drupal-3317769/-/pipelines/201317/test_...

Rebased and rebuilt the ckeditor JS artifacts. Let's see if the test baseline changed. I'll take a stab at some of the non-JS @todos, as well.

bradjones1 → changed the visibility of the branch 3317769-cke5-entity-link-suggestions to hidden.

bradjones1 → changed the visibility of the branch 11.x to hidden.

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

@marcofernandes Commenting on a closed issue is unlikely to get you a response. Also, D9 is unsupported, so the general recommendation will be to upgrade first.

Does this mean we could deprecate the existing lazy proxy creation script and require any new core lazy services be injected with a service closure or something else less boilerplate?

Re: ##13, we're using this pattern at 🐛 Cache bin names should be set from service tags, not the service name Needs review though as mentioned, the Drupal container doesn't support it directly. I disagree that the Symfony syntax is too ugly to use, but service closure is a good candidate here to remove all this even uglier boilerplate.

but what does not make sense (to me, yet) is that the issue summary talks about how this is not for the field level, yet this issue's MR is introducing two new (test-only) field types: see the JsonBackedPropertiesItem and MappedPropertiesItem classes. That seems a contradiction at first sight?

Re: #71 - the big difference between this feature-set and what you have in json_field is querying. As you can see from their "native" field types, it's rather trivial to tell your database to create a JSON(B) column and stick some JSON in it. But, you can't query nested values. As Wim's linked issue reveals... that's kinda the point of storing data as JSON, otherwise it's just another string. And as this issue's long history illustrates, that's not trivial. But it does unlock a lot of potential to be able to straddle the line between relational database models and JSON documents where you can query arbitrarily-nested values directly. (Yes, I know it may be less performant without an index, but those are also easy to make and querying with a table-scan isn't the worst sin in the world for most sites.)

This issue doesn't ship with a field type out of the box because, IMO, that's rather boring - most end-users aren't eager to edit JSON on the front end. It's anticipated that the majority of use-cases for this early on would be things like experience builder or layout builder or your-custom-field that contains a JSON document you get from wherever, e.g. an API. This is expert-mode stuff that enables novice stuff through a specific implementation. That's why the map item is a test fixture.

Coding standards bumped, keeping RTBC.

OK, marking this NR.

Digging into this a bit, I discovered/decided a few things:

• Most of Drupal core expects that you'll really only have one default target and (maybe) a replica target. Non-replica targets for a database key, especially default, aren't anticipated in production and this results in a required bugfix here. Basically, the assumption was that if there were more than one target, it must be a replica, and thus we need to disable replicas during certain actions. This was discovered during a regression surfaced in testing where the session was re-started due to the replica kill switch service.
• Speaking of replicas, we use that term a lot around the DB API and it's not entirely clear that replica is a reserved word that specifically designates a replica or group of replica connection targets. It was particularly difficult to find all instances of this "DB replica" vs. other uses of the word. I think constants are a great answer to this question, so in the course of fixing above bug in adding this functionality, I extracted all these words into constants.
• I landed on a decorator pattern for the non-transactional connection, which wraps the underlying driver's connection object. As I have navigated in the course of my work on JSON data support, messing with signatures for the various connection classes and interfaces is no small feat, and there are also contrib drivers to think about. Decoration is a good solution here, but as you can see it results in a lot of boilerplate. I marked this class both final and internal, and it did still require a change broadening the potential return types of connection methods. (see the event enable and disable methods.) As I expect this to really be an internal API (there's little to no reason to override the non-transactional decorator class) I'm hoping this is sufficient. Otherwise we need to add a bunch of code to the connection classes that boils down to throwing an exception if you try and start a transaction on this connection. I don't think that's worth it.
• This also required a few tweaks to performance testing (though with no real change in performance) due to there now being two connections always in play - transactional and non.

bradjones1 → created an issue.

For anyone losing their mind over why this isn't working lately, see

https://git.drupalcode.org/project/gitlab_templates/-/commit/d814dac179d...

which prefixes chromeOptions with goog:

My thought would be to deprecate it as part of implementing 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed with aggressive removal soon thereafter (might be able to do so in a minor, even, if we stick to our guns regarding it truly being internal.)

Re: 193, see #3451460-11: Sessions are invalidated on subsequent page request when written while transaction is open → for some next steps on that exact problem.

Well... it turns out this is basically a dupe of 🐛 Transaction autocommit during shutdown relies on unreliable object destruction order (xdebug 3.3+ enabled) Needs review .

Discussion in Slack is that this is indeed insidious and would be avoided in general by using a separate, non-transactional database connection a la 🐛 Race conditions with lock/cache using non-database storage - add a non-transactional database connection Needs work and deprecating/removing non-explicitly-committed database transactions at 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed

Closing Won't Fix (Fix Elsewhere.)

I am curious if this is an edge case with TransactionManagerBase. In my case, two transactions that are not destroyed seem to fall under this docblock in ::unpile():

    // If the $id does not correspond to the one in the stack for that $name,
    // we are facing an orphaned Transaction object (for example in case of a
    // DDL statement breaking an active transaction). That should be listed in
    // $voidedItems, so we can remove it from there.
    if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
      unset($this->voidedItems[$id]);
      return;
    }

That got me looking at $this->voidedItems, which is empty. So these were never "voided." While these transactions were never added to the voided item list, the docblock in ::voidStackItem() says:

    // The item should be removed from $stack and added to $voidedItems for
    // later processing.

There isn't a read accessor of $voidedItems that I can find - did I just discover a bug in the transaction manager, or am I just not familiar enough with this to know what I'm looking at?

Re: #8 - At first I was like, duh, yes it would be destroyed when going out of scope, I was barking up the wrong tree.

Except then, I tried to go in and reconcile each transaction "push" with a destruction. Each push was from ::save(), however I had two that were not cleaned up. I even added an explicit unset($transaction) before the return and same result - there were two objects that were only destroyed later during shutdown.

I think this is at least in part due to the fact I am doing an entity save during an entity insert hook - e.g., I create a profile entity for the newly-created user. I suspect this is related somehow. There's no other reference to the Transaction object returned from ::push(), so I'm kinda mystified. PHP must not be allowing the reference to be destroyed, even when I explicitly unset() it in ::save(). Does this ring a bell to anyone?

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

Thanks for chiming in.

I think the question here is not even related to implicit vs explicit commit, but rather why should a transaction be active at this point.

Perhaps the question isn't directly implicit vs. explicit, but since Drupal currently supports implicit transaction commits/completion, then why wouldn't there be a transaction active?

I dropped a breakpoint on TransactionManagerBase::push() and found that the open transaction was started in SqlContentEntityStorage::save(), where it is not explicitly destroyed. So core is opening the transaction. What's more, the object isn't returned to the caller so there's no way to explicitly commit. If you follow the instructions on the transaction docs, then yes, you could start a transaction, then ::save() would add a savepoint, and then the transaction could be explicitly committed. But that would mean a pretty significant shift for developers to always have to wrap their entity saves in transactions in order to avoid the condition that prompted this issue.

That also explains why I only experience this bug after a user is created, because of the entity save operation.