Change Sorter callable to class

[pulzarraider]

Q	A
Type	improvement
BC Break	yes
Fixed issues	-

Summary

This PR replaces the custom callable for sorting migrations with the class implementing new interface Doctrine\Migrations\Sorter.
It can help to define different sorting algorithms not just using the function uasort. I think it will be more universal for some edge cases.

It can be merged to master (prepared v3) which has not been tagged yet.

[greg0ire]

Should we maybe use __invoke here? Not sure what others think about that…

[goetas]

First of all, thanks for contributing! Especially to doctrine/migrations 3.x that still has to be released! You are awesome!

I think it will be more universal for some edge cases.

Can you explain the reasons behind your PR? What are the main advantages over a callable? (consider that if applying @greg0ire's suggestion in #902 (comment), it will be exactly a callable..)

[pulzarraider]

Thank you @goetas.

Doctrine migrations 1.* and 2.* supports only one directory with migrations which was not enough for our use case. Few years ago we created ecosystem of internal bundles/libraries each with own migrations (and database tables) to simplify development process of our projects (many projects has similar features). The bundle migrations live together in the git repository of the bundle and is found automatically and run together with other migrations during the deployment process. So developer doesn't have to care of tables and their migrations of bundle which is developing by the other team.

Thanks to great people working on Doctrine project it's now possible to drop our internal migrations solution and replace it with Doctrine migrations v3.

But some features are still missing, I will create more PRs to improve the migrations so we can replace our solution.

For this PR:
In our ecosystem of bundles the order in which the migrations run is important because some migrations depend on each other. The main change of this PR is to allow to modify the sorting algorithm for migrations to be more complex, not just using the uasort function. I do not care if the class will have __invoke or sort method. I'll change it according to your preference.

[stof]

If we go the way of expecting a sorter service performing the full sorting rather than expecting a comparison callback, I would at least provide a core implementation of the interface expecting a comparison callback, to make the migration easier.

Btw, what kind of sorting algorithm do you have, so that it cannot be implemented as a comparator passed to uasort ?

[pulzarraider]

Good point @stof, I can create it.

Btw, what kind of sorting algorithm do you have, so that it cannot be implemented as a comparator passed to uasort ?

Recursive sorting. Maybe it can be implemented with uasort, but some preprocessing is required (to find parent and dependent migrations), because it cannot solve the order with just comparing two versions.

[pulzarraider]

Small performance question:
Do you know why the uasort is called each time the migration is added to the list (inside registerMigrationInstance)? Sorting the migrations just once when the list is finished in loadMigrationsFromDirectories() will be much faster.

[goetas]

Recursive sorting. Maybe it can be implemented with uasort, but some preprocessing is required (to find parent and dependent migrations), because it cannot solve the order with just comparing two versions.

This can be implemented without any issue with the current callback system. This is an example implementation using complex dependency graph resolution.

use Doctrine\Migrations\Metadata\AvailableMigration;
use MJS\TopSort\Implementations\StringSort;

class MigrationSorter
{
    private $deps = [];

    private function buildDeps()
    {        
        $sorter = new StringSort();
        
        $sorter->add('package-a', ['package-c', 'package-b']);
        $sorter->add('package-c', ['package-b']);
        

        $this->deps = array_flip($sorter->sort());

    }

    public function __invoke(AvailableMigration $a, AvailableMigration $b):int
    {
        if (empty($this->deps)) {
             $this->buildDeps();
        }


        $nsA = findPackage((string)$a->getVersion());
        $nsB = findPackage((string)$b->getVersion());
        return $this->deps[$nsA] <=> $this->deps[$nsB];
    }

findPackage can be any function that is able to map a namespace to a package you have.

[pulzarraider]

@goetas Yes, this is excatly what I said as the preprocessing - creating sorted list which will be used for uasort. You are sorting it twice now. Fisrt with MJS\TopSort and then with uasort.

Your example can be simplified replacing the uasort with simple foreach to fill the sorted array with migration instances:

class MigrationSorter
{
    private $deps = [];

    private function buildDeps(): void
    {
        $sorter = new StringSort();

        $sorter->add('package-a', ['package-c', 'package-b']);
        $sorter->add('package-c', ['package-b']);


        $this->deps = array_flip($sorter->sort());
    }

    public function sort(array &$migrations) : void
    {
        if (empty($this->deps)) {
            $this->buildDeps();
        }

        foreach ($this->deps as $key => &$value) {
            $value = $migrations[$key];
        }

        $migrations = $this->deps;
    }
}

There is no need to use the uasort function if you use some other more complex library for sorting.

[goetas]

The reason why I did it with uasort was also to avoid that users might inject more migrations in the array (as it is by reference) in this case). Uasort obliges to focus only on sorting.

MJS\TopSort is resolving package dependencies, not sorting migrations. Those are two different processes.

From what I know Sylius might need this. Do you know anyone from their team to give an opinion here?

[pulzarraider]

If we go the way of expecting a sorter service performing the full sorting rather than expecting a comparison callback, I would at least provide a core implementation of the interface expecting a comparison callback, to make the migration easier.

Implemented. Thank you @stof

The reason why I did it with uasort was also to avoid that users might inject more migrations in the array (as it is by reference) in this case). Uasort obliges to focus only on sorting.

@goetas You're right that's more safe. If possible injections of migrations are problem, we can close this PR. I have no good idea how to prevent this happen.
We can create SplObjectStorage with migrations before the sorting and compare sorted array with this list. If some new migration will be found, we will throw the exception. But that will cost more time and memory.

Small performance question:
Do you know why the uasort is called each time the migration is added to the list (inside registerMigrationInstance)? Sorting the migrations just once when the list is finished in loadMigrationsFromDirectories() will be much faster.

I have moved the sorting operation to loadMigrationsFromDirectories to sort the migrations just once. If this PR will be closed. I will create new one with just this performance optimization.

From what I know Sylius might need this. Do you know anyone from their team to give an opinion here?

No, but we can ask. @pamil @Zales0123 @Tomanhez @AdamKasp @GSadee Do you have some use case for this PR?

[goetas]

What about an interface as:

interface Sorter 
{
    public function __invoke(AvailableMigration $a, AvailableMigration $b) : int;
}

?
This will give type-safety and allow to extract the default implementation in a separate class.

[goetas]

#910 is my proposal.

(that solves another issee that I did not notice before).

This change is needed if we want to be able to sort executed unavailable migrations. Being able to sort unavailable migrations is important to allow to implement properly #908 by offering all the migrations in a single table instead of two separate.

I have moved the sorting operation to loadMigrationsFromDirectories to sort the migrations just once. If this PR will be closed. I will create new one with just this performance optimization.

I have applied this performance optimization in #910

[pulzarraider]

Thank you @goetas. I am ok with the solution in #910. Great that you applied the performance optimization I proposed.