Moved migration sorting from MigrationRepository to MigrationPlanCalculator

[goetas]

Q	A
Type	improvement
BC Break	-
Fixed issues	-

This change will allow to define sorting by looking into the available migrations array. Before this change was not possible as the same object responsible for sorting migrations, was responsible also for locating migrations (this the circular dependency..).

[goetas]

Thanks for the PR.

@riccardonar You can fix CS failures running vendor/bin/phpcs.

[goetas]

This pull request brings an interesting point. Are executed migrations sorted? if yes, how? using the same sorting for available migrations?

if they are not sorted, ExecutedMigrationsSet should not have the getLast and getFirst methods, but then it becomes hard to to find the current migration status

[riccardonar]

This pull request brings an interesting point. Are executed migrations sorted? if yes, how? using the same sorting for available migrations?

if they are not sorted, ExecutedMigrationsSet should not have the getLast and getFirst methods, but then it becomes hard to to find the current migration status

Couldn't ExecutedMigrationsSet be sorted by the time it was recorded as executed on db?
Certainly the ordering between executed and available could change

[goetas]

This pull request brings an interesting point. Are executed migrations sorted? if yes, how? using the same sorting for available migrations?

if they are not sorted, ExecutedMigrationsSet should not have the getLast and getFirst methods, but then it becomes hard to to find the current migration status

Couldn't ExecutedMigrationsSet be sorted by the time it was recorded as executed on db?
Certainly the ordering between executed and available could change

#974 will solve that

[goetas]

This is ready for review!

[greg0ire]

Since we have array_values…

Suggested change

	/** @var AvailableMigration[] */
	/** @var list<AvailableMigration> */

[goetas]

Is this compatible with common IDEs ?

[greg0ire]

It's compatible with vim :P

[greg0ire]

Alternative suggested here:

Suggested change

	/** @var AvailableMigration[] */
	/**
	* @var AvailableMigration[]
	* @phpstan-var AvailableMigration[]
	* @psalm-var AvailableMigration[]
	*/

Not that the second one is superflous: both tools read each other's annotations

[goetas]

I personally do not like this "tool" specific annotations... if a third tool will emerge will need to add them too... i suggest to leave AvailableMigration[] as it is

[greg0ire]

Suggested change

	* @return AvailableMigration[]
	* @return list<AvailableMigration>

[greg0ire]

Maybe we should discuss as a team what we want regarding annotations, anyway it's not a good reason to block this PR.

[goetas]

I'm merging this. The conversation about list or [] can be discussed again in the future at it will impact the whole codebase, not only the changes in this PR.