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

Allow behavior of getMigrations() method in CakeManager to be modified or overridden #740

Open
zejji opened this issue Aug 22, 2024 · 7 comments

Comments

@zejji
Copy link

zejji commented Aug 22, 2024

Description

We maintain a large CakePHP 4.4 application with many hundreds of thousands of lines of code. In recent years, we have increasingly moved towards a trunk-based development model, i.e.:

  • we avoid long-lived branches;
  • in-progress code is merged regularly into the main branch;
  • feature flags are used to render in-progress features inert during development.

Overall, trunk-based development works well for us. However, the one problem we have with trunk-based development when using CakePHP relates to migrations.

Specifically, we want to be able to only run certain migrations if a related feature flag is enabled.

We have tried using feature flags within the up/down/change methods themselves, but this does not work, because the migration itself is still marked as migrated once bin/cake migrations migrate has been run. This leads to the migration not being run again when the feature flag is ultimately enabled.

Furthermore, it's not possible for us to override the Phinx Manager class which contains the getMigrations method, or the CakeManager class which overrides it, as these classes are instantiated via the new keyword. In other words, dependency injection is not used within the CakePHP framework code - it is tightly coupled to the concrete Manager and CakeManager classes.

Please could you consider providing a mechanism by which the behavior of the getMigrations() method in CakeManager can be modified/overridden/extended.

CakePHP Version

4.4

@ADmad ADmad transferred this issue from cakephp/cakephp Aug 22, 2024
@markstory
Copy link
Member

Having a custom migration manager that skips migrations without running them could cause issues elsewhere in the plugin as there are some assumptions around migrations being applied in the order they are defined/added.

Making the manager created through DI could be an option with the new built-in backend implementation that will be available for cake5 applications, but there aren't any plans to backport that to 4.x compatible migrations releases.

Specifically, we want to be able to only run certain migrations if a related feature flag is enabled.

Why? additive schema changes can be done without feature flags, and destructive schema changes are generally not compatible with feature flag based development as feature flags can be turned on an off outside of deploys and thus you would need schema that is compatible with both sides of each feature flag.

@zejji
Copy link
Author

zejji commented Aug 27, 2024

@markstory - Thanks for getting back to me.

In answer to your question why we would only want to run certain migrations if a feature flag is enabled - this is because some of our migrations add 'system data' as opposed to making schema changes. By 'system data' I mean data that is required in all environments in order for the application to run.

Where data is added by a migration, such changes are generally not backwards-compatible and will affect the operation of the system, which is why we want to be able to defer the addition of the data until the associated feature flag is enabled. An example of such 'system data' might be default permissions associated with a new feature. We have many hundreds of such migrations in our system over the years.

When we write such migrations, we can easily design them in such a way that it does not matter if the feature flag is enabled much later than the original time of creating the migration. Furthermore, we have automated test pipelines that can run the migrations with both the feature enabled and disabled, ensuring that both options work correctly.

Update:

Just to clarify - for our purposes it would be sufficient to have the ability to either: (i) make migrations that require the feature flag invisible to the migrations commands; or (ii) skip such migrations without making them invisible.

@markstory
Copy link
Member

In answer to your question why we would only want to run certain migrations if a feature flag is enabled - this is because some of our migrations add 'system data' as opposed to making schema changes. By 'system data' I mean data that is required in all environments in order for the application to run.

Interesting, I've not worked on a system that operated this way. Having feature flagged code manipulate existing application configuration sounds challenging.

make migrations that require the feature flag invisible to the migrations commands

I can think of two possible solutions with existing behavior that might cover your scenario:

  1. Migrations have a shouldExecute hook method that you could use to 'skip' migrations that don't have feature flags enabled. If a migration returns false from shouldExecute it takes no action and prints out a message noting that the migration was skipped.
  2. You could use multiple 'sources'. Within config/Migrations you can create additional directories of migrations for your feature flags. For example you could have config/Migrations/BlueSidebar/20240828123045_add_stuff_for_blue_bar.php. Once the migration is ready to be run it can be moved to config/Migrations to be run as part of the mainline migration history. Would that give you enough control to make migrations that require feature flags invisible? You would only run migrations for the feature flags that you have migrations enabled.

@zejji
Copy link
Author

zejji commented Aug 30, 2024

@markstory - The shouldExecute hook in particular sounds as though it would be very useful!

This would give the flexibility to handle feature flags as well as other scenarios requiring conditional migrations.

For example, our codebase is also deployed to multiple sites, some of which require different 'system data' migrations. Again, these could be handled by a shouldExecute hook. This would be neater than having to apply an environment check in both the up and down methods.

@markstory
Copy link
Member

Cool. Let me know how it goes using shouldExecute(). It is existing behavior, and should work today.

@zejji
Copy link
Author

zejji commented Sep 3, 2024

@markstory - Many thanks.

Do you happen to know when shouldExecute was added? We're currently on CakePHP 4.3 (upgrading to 4.4 imminently).

@markstory
Copy link
Member

shouldExecute was added to phinx in cakephp/phinx@da9474e which was first part of 0.13.0

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

No branches or pull requests

2 participants