Locking reads

[prgTW]

This PR adds ability for locking reads, either shared or exclusive

Compatibility with:

• MySQL 5.6+
• PostgreSQL 8.1+

Example:

$booksReadInShareMode = (new BookQuery)
    ->filterById(123)
    ->forShare()
    ->find();
// SELECT ... FROM book LOCK IN SHARE MODE -- mysql
// SELECT ... FROM book FOR SHARE -- pgsql

$booksToBeUpdated = (new BookQuery)
  ->filterById(123)
  ->forUpdate()
  ->find();
// SELECT ... FROM book FOR UPDATE

[dereuromark]

https://travis-ci.org/github/propelorm/Propel2/jobs/735176770
You can run the sniffer to auto fix those issues.

[prgTW]

@dereuromark yes, I forgot there are cs-check/cs-fix composer scripts. Already fixed.

[dereuromark]

Do you intend to add more? As your PR is still in draft mode.

[prgTW]

Yes, I can flag it as "ready for review" because, for now, I'm unaware of anything that this feature breaks or to be tested more thoroughly. Of course I'm open for discussion, hints etc..

[prgTW]

I also thought about adding forShare()/forUpdate() instead of withLock to skip passing Lock::SHARED or Lock::EXCLUSIVE explicitly.

Pros:
+ being closer to how we write native SQL query (`SELECT .. FOR SHARE` and `SELECT .. FOR UPDATE`)
+ safety, as one could pass a custom string e.g. `non-existing` as the lock type (or validation should be introduced)

Cons:
- introducing new type of lock (future proof) would require new method

[dereuromark]

The current implementation sounds like a good compromise. I would probably keep it as is for now.

[ehsanmx]

As far as I know, the method name forUpdate is a very standard and common name, and some other projects already implemented this as it was necessary for them, this can cause BC break:
e.g.
https://github.com/spryker/propel-orm/blob/master/src/Spryker/Zed/PropelOrm/Business/Builder/QueryBuilder.php#L447

so maybe a more unique name can reduce the chance of the breaking or do you have a better idea? btw this is the same for forShare

[prgTW]

Hi @ehsanmx. Yes, these names are common-looking because I wanted to make the naming similar to how Propel is built in general. Originally the API was withLock(Lock::SHARED) but that just didn't feel right IMHO, not with other methods named so simple and similar-looking to what SQL offers in standard - that's the "why" part of this PR.

I know that such change could introduce a BC break in other projects but should we look at who and how implemented the library? Choosing other names for these methods like selectForUpdate may subject to the same problem - somebody somewhere may have implemented such method already - it's rather a matter of probability of such situation in other projects. Personally I don't think that it's a BC break because the API isn't changed or removed but instead a feature is added so it's a minor change (according to Semantic Versioning). I don't know if Propel follows semantic versioning (doubt it really) although the idea of following it would be nice.

But, not to be so negative about it 😉, I would propose also lockForShare/lockForUpdate or withSharedLock/withExclusiveLock, however these don't feel as nice and close to native SQL parts as the originally proposed names. Maybe that's the same reason why currently proposed naming was also implemented in QueryBuilder.php#L447 You mentioned?

btw. thanks for the review!

[ehsanmx]

Hey @prgTW,
Thanks for your reply, I think having more keywords in the method name like lockForShare/lockForUpdate reduce the chance of breaking and I'm ok with that

[prgTW]

Naming changed as requested - lockForShare,lockForUpdate,withoutLock :)

[dereuromark]

The PHP 8 compatibility seems to affect this PR now. Requires a resolve now.

[prgTW]

Rebased on newest master and squashed

[prgTW]

Tests are passing but one phpstan error, however I would prefer this error to be fixed in a separate PR no to mix responsibilities

[dereuromark]

Great work!
We should also add docs for it in https://github.com/propelorm/propelorm.github.com