8000 implementing newSubset and unavailableSubset for migration lists by chr-hertel · Pull Request #1174 · doctrine/migrations · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

implementing newSubset and unavailableSubset for migration lists #1174

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.

8000
Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2021
Merged

implementing newSubset and unavailableSubset for migration lists #1174

merged 1 commit into from
Jul 4, 2021

Conversation

chr-hertel
Copy link
Contributor
Q A
Type improvement
BC Break no
Fixed issues doctrine/DoctrineMigrationsBundle#421 (comment)

Summary

As as result of @greg0ire review in doctrine/DoctrineMigrationsBundle#421 I added

  • ExecutedMigrationList->unavailableSubset(AvailableMigrationList)
  • AvailableMigrationList->newSubset(ExecutedMigrationList)

Includes tests and refactoring of CurrentMigrationStatusCalculator

@chr-hertel chr-hertel mentioned this pull request Jul 3, 2021
Merged
@greg0ire greg0ire changed the base branch from 3.1.x to 3.2.x July 3, 2021 12:28
@greg0ire
Copy link
Member
greg0ire commented Jul 3, 2021

I retargeted to 3.2.x since this is a new feature. I'm also working on fixing the build at #1175

@greg0ire greg0ire requested a review from goetas July 3, 2021 20:47
@goetas
Copy link
Member
goetas commented Jul 4, 2021

I'm not 100% sure this is a good idea.

When developing the initial version of the 3.0 release I did something similar (adding some "convenience" methods in the collection classes that were allowing to filter/manipulate data related to that collection).

I've realized that such approach was not working well since such logic sometimes might require external services or uses the collection classes as placeholder for utility methods... and the number of methods in there was growing and growing every commit i was doing.

Said methods have been removed with 60a1d47 and their functionality have been moved to the migrations calculator (that now become a much more important class in the migrations project).

Would it make sense to have a CachedMetadataStorage that decorates the initial Doctrine\Migrations\Metadata\Storage\MetadataStorage offering some cached version of the migrations status? in this way multiple calls to the MetadataStorage instance would lead to only one set of queries (that would fix globally the performance issue we are trying to solve in the bundle...?).

(this is only one possible solution... do you see any other?)

@greg0ire
Copy link
Member
greg0ire commented Jul 4, 2021

IMO, since no "use" statements were added, it means the methods that were added are not out of place. At least they are not requiring external services (but do require external classes in the same namespace).

the number of methods in there was growing and growing every commit i was doing

IMO that's for the best, it's far better to have this than to have "utility methods" in some Util namespace or whatever. Maybe the other methods you describe did not really belong to the collection classes?

I think we should double down and use the newly introduced methods in CurrentMigrationStatusCalculator

Regarding the cache solution, it would involve having an available cache pool, or should we force the usage of a per-http-request cache?

@goetas
Copy link
Member
goetas commented Jul 4, 2021

IMO, since no "use" statements were added, it means the methods that were added are not out of place. At least they are not requiring external services (but do require external classes in the same namespace).

the number of methods in there was growing and growing every commit i was doing

IMO that's for the best, it's far better to have this than to have "utility methods" in some Util namespace or whatever. Maybe the other methods you describe did not really belong to the collection classes?
I think we should double down and use the newly introduced methods in CurrentMigrationStatusCalculator

I do not have a strong opinion here, it was just my past experience...
If we start adding this kind of methods in collection classes, then the methods in CurrentMigrationStatusCalculator in most of the cases will just proxy the call to the collection class... a downside of such approach is that in the future it will not be clear with method is the source of truth? should the user rely on the "util methods" added in the collection classes or to rely on some other service provided by the DI container having similar or same methods?

I'm ready to try it again...

Regarding the cache solution, it would involve having an available cache pool, or should we force the usage of a per-http-request cache?

My idea here was just to have some sort of instance/array cache that stores the computation for the same class instance, that was it it.

@greg0ire
Copy link
Member
greg0ire commented Jul 4, 2021

The fact that we need code similar to what's in CurrentMigrationStatusCalculator in the bundle should be a good indicator that we might instead want to have these methods in the collection classes. Besides, I don't think there is anything wrong with having more than adders, removers and getters in a collection class, quite the contrary: if that was the case, then we would use a generic collection class and use psalm annotations like @template to get some type safety.

a downside of such approach is that in the future it will not be clear with method is the source of truth

If you have one method that proxies to another, then by definition this last one is the source of truth. It's still good to have CurrentMigrationStatusCalculator around though, that way the collection classes do not have to know about MetadataStorage or MigrationPlanCalculator which I believe fall into the "external services" category you mentioned before.

@chr-hertel , do you have any thoughts to share on this? It would be great to have your point of view as well.

goetas
goetas approved these changes Jul 4, 2021
View reviewed changes
Copy link
Member
@goetas goetas left a comment

Choose a reason for hiding this comment

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

@greg0ire your explanation makes sense, I agree with the changes in this pull request.

@greg0ire greg0ire added this to the 3.2.0 milestone Jul 4, 2021
@greg0ire greg0ire merged commit 5586aab into doctrine:3.2.x Jul 4, 2021
@greg0ire
Copy link
Member
greg0ire commented Jul 4, 2021

Thanks @chr-hertel !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

@goetas goetas goetas approved these changes

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@chr-hertel @greg0ire @goetas
0