feat: Allow to unmark an entity as readonly #12167
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
In #7936 the
Query::HINT_READ_ONLYhint was introduced. This allows to fetch entities via Doctrine but without having them to take additional computation resources when computing the changeset in the unit of work.When it was introduced, there was the question of what to do about a model for which the hint is set, but does not match how the entity was initially loaded. In other words, what should happen if you load an entity as readonly when it is already present as not readonly in the unit of work and vice-versa. At that time, the very reasonable decision of not challenging the state was made. In practice, this means if you fetched a entity as readonly and try to use it later, even if explicitly set with the hint readonly
false, it will be kept as readonly (until you clear the unit of work at least).The problem
In practice, I think this is a bit limiting and IMO problematic in two ways.
A potential scenario is various services that may fetch entities for read/validation purposes: they consume the entities, but are not expected to modify them in any way. Fetching models as readonly at this levels allows to "warmup" the unit of work without bloating it later for when computing the changesets.
However, in that scenario, if a sub-sequent service wants to update a model, it cannot do so anymore unless we clear the entire unit of work. It is not ideal.
Another problem, is that any attempt at an update will silently fail.
Possible solutions
In this PR I explored a fix. In the end it ends up introducing a feature which is allowing the entity manager to find an entity as readonly. I.e. if the entity is not in the unit of work yet, it will be fetched with the readonly hint. Otherwise, nothing is done*.
*: this is debatable but I think it is in the spirit of the current implementation of Doctrine towards readonly objects. Indeed, Doctrine doesn't go out of its way to make a entity readonly, i.e. in the end a user can still update the properties of the entity and they will simply be silently ignored during a flush. With that in mind, if you fetch a model as readonly but it was previously fetched as writable, there is no issue IMO.
Thanks to this feature, we can now unmark a model as readonly if it was previously readonly, but fetched as writable.
Now I am aware that even if everyone agrees with this change, the current implementation cannot land as is (either missing some tests or BC breaks), but I think it is better to present the bigger picture first.
If you are not happy with the whole thing, please at least consider being able to unmark an entity as readonly from the unit of work.