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

Add Migrations middleware #774

Merged
merged 10 commits into from
Dec 7, 2024
Merged

Add Migrations middleware #774

merged 10 commits into from
Dec 7, 2024

Conversation

dereuromark
Copy link
Member

@dereuromark dereuromark commented Nov 26, 2024

Resolves #317

We could check on a query string skip-migration-check = 1 and if so, also behave like non debug mode.
This can be useful if you need to check something prior to actually running it locally.

@dereuromark
Copy link
Member Author

dereuromark commented Nov 29, 2024

pending_migrations

Open tasks

  • Make connection table more dynamic (and get default one from connection manager)
  • Make phinxlog table more dynamic
  • tests
  • 'app' false/true to only allow plugins to be checked (e.g. if you are working alone, and only want to check vendor plugins)

@dereuromark dereuromark changed the title Add Migrations middleware WIP: Add Migrations middleware Nov 29, 2024
@LordSimal
Copy link
Contributor

I can think of plugins which do contain migrations which I don't want/need to run in my app because I just use parts of that plugin (e.g. this one).
Therefore it would be good to ignore either a whole plugin from this logic or specific migrations.

docs/en/index.rst Outdated Show resolved Hide resolved
docs/en/index.rst Outdated Show resolved Hide resolved
docs/en/index.rst Outdated Show resolved Hide resolved
src/Middleware/PendingMigrationsMiddleware.php Outdated Show resolved Hide resolved
@dereuromark
Copy link
Member Author

Therefore it would be good to ignore either a whole plugin from this logic or specific migrations.

The plugins list is already opt-in by default, so if you dont specify a plugin it would not run the check on it.
If you skip specific migrations, that is out of scope here.

@dereuromark
Copy link
Member Author

dereuromark commented Dec 2, 2024

It works now for app and plugins, you can provide a list of plugins, otherwise only app.
You can provide a different connection string, otherwise defaults to default.

Anything else? Or does this only need tests etc to get finished up?

Also removed some dead dormant code that wasnt been used for a while.

@dereuromark dereuromark changed the title WIP: Add Migrations middleware Add Migrations middleware Dec 2, 2024
@dereuromark
Copy link
Member Author

I might need a bit of help with the tests, also because they seem to leak to stdout

@dereuromark
Copy link
Member Author

dereuromark commented Dec 2, 2024

Ideas that could also be done as part of a follow up PR

  • 'cache' false/true to allow caching based on the hashsum of all migration files (app+plugin), this would then safe a bit of queries maybe? on the other side this needs still the file lookups.

@dereuromark
Copy link
Member Author

dereuromark commented Dec 4, 2024

@markstory

1) Migrations\Test\TestCase\Middleware\PendingMigrationsMiddlewareTest::testAppMigrationsSuccess
TypeError: Migrations\Db\Adapter\MysqlAdapter::quoteTableName(): Argument #1 ($tableName) must be 
of type string, null given, called in 
/home/runner/work/migrations/migrations/src/Db/Adapter/MysqlAdapter.php on line 110

Can we relax this code not to require the name if not given?
It seems to work fine for other adapters.
Or do we need to fix this up? If so, I wonder how to make the tableName not null.

//EDIT
Actually, I think there is sth weird in the way the Manager is delegating that info.
But I managed to fix up the tests.

@dereuromark dereuromark requested a review from markstory December 6, 2024 01:39
@markstory markstory merged commit e000c3a into 4.x Dec 7, 2024
16 checks passed
@markstory markstory deleted the 4.x-middleware branch December 7, 2024 17:41
@dereuromark
Copy link
Member Author

@markstory The tests leaking some stdout output in console still seems to be there.
I wonder if we can somehow silence this for the middleware?

@markstory
Copy link
Member

In other tests those leaks were caused by new output objects being created instead of using stubs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise exception when there are migrations yet to be run
3 participants