Add new shouldExecute method to prevent a migration or seeder from being executed

[AdrienPoupa]

The company I work for uses Phinx and we are pleased with it. However what's currently stopping us from using it in production is not being able to run migrations in two steps, before and after deployment.

This is an issue that's been raised several times already, in #529, #1044, #1619 and #1824

I started using my own implementation of AbstractMigration and creating a custom template, but I realized that there was a key feature that I was missing if I did not want to wrap all my code in big if conditions in all my migrations: a way to stop the migrations from being executed at the Manager level, before it inserts data in phinxlog. This was rightly suggested here: #1619 (comment)

Even with if conditions as mentioned in #1619, the migration would still be recorded in the database and not tried again. What I needed was a way to make sure it would only be executed when the shouldExecute method returns true.

As I implemented it, a custom AbstractMigration implementation can override shouldExecute and perform any logic it wants to determine if the migration should run.

If not, it displays "skipped" in the console and stops there.

I was toying with the idea of changing how preFlightCheck works, and making it return a boolean and execute the migrations or not based on that, but it would break its existing behaviour, and I was not sure what to do with postFlightCheck, resulting in an inconsistency between the two methods.

While I was at it, I also refactored the console status message display to use a common method to display all the similar migration/seed status messages.

I believe this method would provide a simple, implementation agnostic solution for pre/post deployments and possibly other uses such as determining if a migration should run based on migration attributes, time, tags, environment, etc.

Please let me know if you have any comments! Thank you :)

[dereuromark]

Does this also make #1926 hook not necessary?

[AdrienPoupa]

If I'm not mistaken #1926 is about seeds. But yes I could add the same check in the seed function to keep it consistent

[dereuromark]

Given that we change interface contracts, we also should put this into 0.13 (.next) branch as target

[AdrienPoupa]

This PR is now targeting 0.next and I applied the changes to Seeders.

stickler-ci complains about migrations but I guess it should ignore the namespaces there since most migrations do not have a namespace and none declare the strict types?

[AdrienPoupa]

Then, about #1926 or even the pre/post deployment requests. I was thinking this would enable us to maybe use traits that define a specific behaviour for shouldExecute. For example, I think your use case could done as follows:

trait SeedOnce 
{
    /**
     * @param string $tableName Table name.
     *
     * @return bool
     */
    public function hasData(string $tableName): bool
    {
        $table = $this->getAdapter()->quoteTableName($tableName);
        $countQuery = $this->getAdapter()->query('SELECT COUNT(*) as count FROM ' . $table);
        $res = $countQuery->fetchAll();

        return $res[0]['count'] > 0;
    }

    /**
     * Checks to see if the seed should be executed.
     *
     * Seed only if there is no data in the specified table
     *
     * @return bool
     */
    public function shouldExecute()
    {
        return !$this->hasData($this->seedOnceTable);
    }
}

class CustomSeeder extends AbstractSeeder
{
    use SeedOnce;

    protected $seedOnceTable = 'posts';
}

I know traits often do not have a good reputation but in this case I think they could be handy :)

Of course this would also work in a class.

[dereuromark]

It is even fine to just have the trait add the hasData() convenience method
Having to code

public function shouldExecute()
{
    return !$this->hasData($this->seedOnceTable);
}

is still not too much work :)

[AdrienPoupa]

You're right. However, using it as a trait (that could be bundled here) means that we do not have to add the hasData function to the AbstractSeeder and only have it when necessary.

[AdrienPoupa]

Hello, I wonder if there are any modifications that should be made before it can be merged to the next branch?

[AdrienPoupa]

Hello, I don't want to overstep but do we have an idea as to when it could be merged?

[dereuromark]

@MasterOdin Are you OK with merging this?

[MasterOdin]

I'm fine with this, as it provides a nice generic abstraction over what #1926 was doing that works for both migrations and seeds.

[AdrienPoupa]

Thanks a lot!