Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mongodb changeset #1277

Merged
merged 7 commits into from
Mar 18, 2023
Merged

Fix mongodb changeset #1277

merged 7 commits into from
Mar 18, 2023

Conversation

IonBazan
Copy link
Contributor

@IonBazan IonBazan commented Mar 16, 2022

This PR tries to fix the entity/document listeners by:

  • Relying on Doctrine\Persistence events
  • Using getObject() from the event itself instead of using Adapter
    - Adding getChangeSet() to the adapter itself so it can be customized

The MongoDB ODM part is a bit poorly tested as many classes are final which makes it really difficult to test.

@IonBazan IonBazan changed the title fix mongodb changeset [help needed] fix mongodb changeset Mar 16, 2022
@IonBazan IonBazan changed the title [help needed] fix mongodb changeset Fix mongodb changeset Apr 26, 2022
@IonBazan
Copy link
Contributor Author

@garak I think this is now ready to go - just the PHPStan errors needs some fixing but most of it doesn't seem to be related to this PR itself.

Copy link
Collaborator

@garak garak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we forced to change an interface? If so, we need to update the changelog with proper instructions. If approved, this change can't be published before the next minor version.
Moreover, the errors reported by phpstan look related to proposed changes.

@IonBazan
Copy link
Contributor Author

Unfortunately, this AdapterInterface change is mandatory:

  • getObjectFromArgs() was removed because we can rely on getObject() on the event itself so the method becomes obsolete
  • getChangeSet() was introduced to provide an adapter between MongoDB ODM (getDocumentChangeSet()) and ORM (getEntityChangeSet()) which is the core of the issue solved by this PR

If we really don't want to touch the interface, we could shift this logic to the listener itself but that would defeat the purpose of using the adapter, especially that the adapter is marked as @internal since v1.19.0.

I will take a look at PHPStan errors again and solve them in next commit 👍🏻

@@ -1,9 +1,9 @@
parameters:
ignoreErrors:
-
message: "#^Method Vich\\\\UploaderBundle\\\\Adapter\\\\AdapterInterface\\:\\:recomputeChangeSet\\(\\) has no return type specified\\.$#"
message: "#^Call to an undefined method Doctrine\\\\Persistence\\\\ObjectManager\\:\\:getUnitOfWork\\(\\)\\.$#"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is ignored because the object returned from getObjectManager() is an instance of Doctrine\ODM\PHPCR\DocumentManagerInterface (https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/DocumentManagerInterface.php) but we are not installing this package so phpstan can't analyze this.

@garak
Copy link
Collaborator

garak commented Apr 27, 2022

@IonBazan I'm not against the interface change, but this PR should include an update to changelog

@garak
Copy link
Collaborator

garak commented Jul 21, 2022

Any news?

@IonBazan
Copy link
Contributor Author

IonBazan commented Aug 1, 2022

@garak sorry, I think the issue that this PR was initially trying to fix has been solved in v1.19.1. I will rebase it and simplify the interface changes then.

@IonBazan
Copy link
Contributor Author

@garak I think this is now ready for a review again. Let me know if the changes still make sense after the changes from v1.19.
I've also added some UPGRADING.md notes to highlight the changes in AdapterInterface.

@garak garak changed the base branch from master to 1.x September 4, 2022 14:28
@garak
Copy link
Collaborator

garak commented Sep 13, 2022

Do you mind proposing the same changes on the master branch?

@IonBazan IonBazan mentioned this pull request Mar 7, 2023
1 task
@garak garak merged commit 8655c36 into dustin10:1.x Mar 18, 2023
@IonBazan IonBazan deleted the bugfix/changeset branch March 20, 2023 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants