Add lockMode to EntityManager#refresh()

[michnovka]

Attempt to implement #10039

[michnovka]

TODO: if accepted, update https://www.doctrine-project.org/projects/doctrine-orm/en/latest/reference/transactions-and-concurrency.html to reflect another case to specify LockMode

[michnovka]

Are there any cases against adding the lock mode into EntityManager#refresh() ?

[michnovka]

@derrabus is there anything I can do to have this merged into 2.14? Anything missing from this PR?

[derrabus]

I really wish I had more time to review changes like this one, sorry. 😞 I'm adding it to the 2.14.0 milestone, so we don't forget it this time.

One thing though: adding parameters to public methods that are neither final nor internal is a breaking change because that would break code that overrides those methods. However, you could use func_get_arg() to access extra parameters passed to a method.

[michnovka]

@derrabus well using func_get_args is such an ugly hack that I would prefer to postpone this to 3.0 rather than try to force this feature thru such obscure parameter hiding.

[derrabus]

Not an option: we don't accept features for 3.0.x.

[michnovka]

you mean 3.0 is under feature freeze already? or it is too soon yet?

[michnovka]

@derrabus I would then suggest we add EntityManager#refreshAndLock() and UnitOfWork#refreshAndLock(). The func_get_args solution is really sth Id like to avoid.

[derrabus]

you mean 3.0 is under feature freeze already?

No, 3.0 is not a feature release. It won't ship a single new feature.

The func_get_args solution is really sth Id like to avoid.

I don't share that sentiment, but I'm open to discussing your solution.

[michnovka]

I don't share that sentiment, but I'm open to discussing your solution.

This is your project, so I am going to implement it with your standards in mind. I just prefer to make BC break in some bigger version release. What versions would be allowed to do that? 2.14?

I checked doctrine code and I don't see such hack being used elsewhere, or do you have some example? If this is not used elsewhere to avoid BC breaks, then I really dont want to be the one blamed for starting it in the future. We can ask others also for their opinion, @beberlei @greg0ire ?

If the version which would allow changing signature of the 2 public methods EntityManager#refreshAndLock() and UnitOfWork#refreshAndLock() were to be too far in the future, I still propose we do now refreshAndLock(), mark it deprecated from the start and when the time comes, add the param to the refresh() function itself.

[greg0ire]

Funny you mention that kind of hack, we got asked about it today: doctrine/dbal@16efd09#r87189625

Personally, I'm fine with it. In any case, please don't contribute BC-breaks to ORM 3 without also providing an upgrade path in ORM 2.

[michnovka]

@greg0ire what is the proper upgrade path for public method signature change? It seems to me the only way to do this is to just do it and then mention it in UPGRADE.md ... There is nothing like #[SignatureWillChange] of the likes of #[ReturnTypeWillChange] or #[Deprecated]

[greg0ire]

One proper way I can think of is using func_get_args(). The other way is to add another method with a different name and deprecate the first one in favor of the second one, but then you loose the name.

[derrabus]

I just prefer to make BC break in some bigger version release. What versions would be allowed to do that? 2.14?

Do you want to contribute a feature or do you want to break things? Contribute your feature in a backwards-compatible manner to 2.14. If the feature is accepted, you can clean up BC hacks in 3.0 after your feature has been merged.

[michnovka]

I have pushed commit which doesnt break BC.

If the feature is accepted, you can clean up BC hacks in 3.0 after your feature has been merged.
Ok, this is what I didnt get, I thought we wont be allowed to change func signatures even in 3.0

When this is merged and changes synced into 3.x, I will commit to the 3.0.x branch to change the signature and remove func_get_args, is that OK?

[derrabus]

When this is merged and changes synced into 3.x, I will commit to the 3.0.x branch to change the signature and remove func_get_args, is that OK?

[derrabus]

Thank you. Please update EntityManagerDecorator as well. We should also update EntityManagerInterface, indicating the upcoming change of the method signatures.

Also, we consider the EntityManager class itself as final, so updating the signatures of public methods would be acceptable there.

[michnovka]

I am on vacation till 7th Nov, can u make the changes yourself or it requires my commits?

[derrabus]

We're not in a hurry here. Do it whenever you feel like you have time. 🙂

[michnovka]

Also, we consider the EntityManager class itself as final, so updating the signatures of public methods would be acceptable there.

@derrabus so can I get rid of the func_get_args code and change the signature as in the original commit? Keeping this workaround for UnitOfWork though as its not final.

[michnovka]

Thank you. Please update EntityManagerDecorator as well. We should also update EntityManagerInterface, indicating the upcoming change of the method signatures.

Neither EntityManagerInterface nor EntityManagerDecorator have the refresh method. Should I add it?

[derrabus]

so can I get rid of the func_get_args code

Yes, but only on the entity manager.

Neither EntityManagerInterface nor EntityManagerDecorator have the refresh method. Should I add it?

They do, inherited from ObjectManager / ObjectManagerDecorator!

[derrabus]

@derrabus I have added the refresh() method to EntityManagerDecorator but not EntityManagerInterface.

You can add a @method annotation to the interface that documents the new signature. You should already find other examples there.

If we want to add the lockMode to the interface, we would need to edit the ObjectManager interface from persistence

No. We simply add the method including the new parameter in 3.0. No need to change Persistence.

[michnovka]

No. We simply add the method including the new parameter in 3.0. No need to change Persistence.

The interface EntityManagerInterface extends Doctrine\Persistence\ObjectManager. The ObjectManager defines

    /**
     * Refreshes the persistent state of an object from the database,
     * overriding any local changes that have not yet been persisted.
     *
     * @param object $object The object to refresh.
     *
     * @return void
     */
    public function refresh(object $object);

How can we change method signature in EntityManagerInterface without changing the persistence? If the signature of a method of a child interface does not match the parent, it wont run. Correct me if I'm wrong.

[derrabus]

Correct me if I'm wrong.

Sure: https://3v4l.org/9EuXT

[michnovka]

@derrabus aha, I didnt realize that by using default parameter value it can work. Thanks.

I have implemented all feedback, this should be ready for a merge.

[derrabus]

Looks good to me. Can you please check if the documentation needs an update?

[michnovka]

@derrabus docs updated

[derrabus]

Thank you.

[michnovka]

@derrabus let me know when you merge this into 3.0.x, Ill work to update the signatures on that branch then

[derrabus]

FTR, I've pushed 474f76f. We usually don't baseline Psalm errors from BC layers.

[derrabus]

Merged up via #10195

[michnovka]

@derrabus should I update stubs at https://github.com/psalm/psalm-plugin-doctrine/tree/2.x/stubs ?

[derrabus]

You should direct this question at the authors of that plugin, I guess. I won't stop you. 🙂