[develop] Adds L11 support

laravel / pennant

A simple, lightweight library for managing feature flags.

https://laravel.com/docs/pennant

MIT License

474 stars 48 forks source link

[develop] Adds L11 support #78

Closed nunomaduro closed 9 months ago

nunomaduro commented 9 months ago

This pull request adds L11 support to pennant.

timacdonald commented 9 months ago

@nunomaduro won't renaming the migration break existing applications that have already run the migration?

The app will see a migration it has not yet run and attempt to run it. We might need to revert that name change.

nunomaduro commented 9 months ago

@timacdonald This pull request is not complete. However, I don't understand your comment. Can you elaborate? The case you described, only would happen if you re-publish your migrations, right?

driesvints commented 9 months ago

~@nunomaduro no, migrations are run when the package is loaded. They aren't "published". Tim's concern (which is valid) is that if this PR gets merged, the app will not recognise the new migration and run it again and thus run into an issue with existing apps because the table already exists. It's different from the skeleton where the migrations exist inside the application.~

nunomaduro commented 9 months ago

@driesvints That only happens if the package is auto-running migrations via the "loadMigrationsFrom" method. That's not the case in pennant, so we can safely up-to-date the migration name's date.

driesvints commented 9 months ago

Seems Pennant is not using loadMigrationsFrom which I thought it did so that rename change should be fine indeed 👍

timacdonald commented 9 months ago

Sorry, folks, I'm an idiot.

I thought Pennant was setup to implicitly add it's migrations to the apps. You are right that you have to publish the migration to be able to run it.

You would think, having written the package AND the docs, that I would know that about Pennant 🙈