Hey!

Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "7.1 for features / 5.4, 6.4, or 7.0 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

I think you can simply revert#50761:

• ArrayIterator won’t work as expected when passed a traversable (this is whytestPostGenerateSchemaWithInvalidLockStore passes: its iterator won’t yield anything. You can be sure because its generator uses$this while beingstatic).
• Exceptions thrown inside a generator will appear when callingvalid ornext, notcurrent.
• An exception thrown inside a generator would close it, even if caught when iterating; this could result in other stores being silently ignored.
• LogicExceptions are not supposed to be caught.

/cc@alexandre-daubois FYI

Indeed my solution doesn't work as expected. I don't think that reverting the PR is the best solution, because it would fix a bug but reintroduce one in the codebase.

I'm on my phone right now but I can have a look at it tomorrow to find/have a look for a proper solution, if you need help 🙂

I think there could be a better solution to the original issue#50761.

Personally, I would take a similar path to Monolog'sNullHandler or Mailer'sNullTransport and introduce aNullStore and a corresponding case to theStoreFactory. The DSN could look something likeLOCK_STORE=null://.

This way we would avoid theInvalidArgumentException, which I agree is a legitimate exception, but preserve the option to effectively disable the Lock component.

Seems to be a great solution indeed, I like it!

I pushed a draft of my proposed solution into this PR, hopefully I didn't miss anything.

Since theStoreFactory already has cases which are not strictly a dsn, I went with a simplenull, which would theoretically work with existing cases ofLOCK_STORE=null.

So, that's a new feature now, isn't it?

Not really a new feature. The current implementation is broken. It will not add the required schema as described in#54406. Just faced the same issue and this PR fixes it :)

TheNullStore is a new feature.

#50761 revert is a fix.

@xabbuh there was in fact no bug prior to#50761: it is normal an exception is thrown when giving theStoreFactory an invalid DSN, and it is outlined by the fact said exception extendsLogicException, which is not supposed to be caught.

OK, so where do we go from here? Keep the current bug in 6.4, revert the commit that causes the bug and having the issue it tried to fix again or something else?

I would be okay with both solutions. Either reverting#50671 or switching toforeach). But the newnull schema cannot be merged into 6.4 and must be moved into a PR targeting 7.2.

I am closing this PR, as it is superseded by#57944.
I am actually only interested in the bugfix part of this issue, so we can remove the workaround after update.
If someone wants to make a PR for the new feature introduced, feel free.