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

Official Laravel integration? #1

Open
MichelJonkman opened this issue Oct 31, 2024 · 15 comments
Open

Official Laravel integration? #1

MichelJonkman opened this issue Oct 31, 2024 · 15 comments

Comments

@MichelJonkman
Copy link

Hey there, I made a similar package though it's not nearly as stable and well maintained as this package. I was just wondering whether there is any official Laravel integration, or if it's planned? Ideally it would include an API that works mostly like Laravel since that would make migrating to Upscheme from Laravel migrations or my own package very easy. If not I will probably do so myself but I don't have the capacity to maintain it for other users.

@aimeos
Copy link
Owner

aimeos commented Oct 31, 2024

We would love if you can create a Laravel integration for Upscheme to be used as alternative to Laravel migrations :-)

@MichelJonkman
Copy link
Author

Bonus question: I also wrote a schema generator that creates migration files based on existing tables, is this something you are interested in as well? Again, I will likely not support it for anything beyond what I personally use but I can create a separate repository for it which can hopefully be expanded upon by others!

@MichelJonkman
Copy link
Author

We would love if you can create a Laravel integration for Upscheme to be used as alternative to Laravel migrations :-)

See the code in https://github.com/MichelJonkman/laravel-declarative-schema for how I've currently done it. Note that this was made pretty quickly so the code quality isn't very good. A key feature of it though, is that normal migrations continue to work, meaning that packages that include migrations keep working.

@aimeos
Copy link
Owner

aimeos commented Oct 31, 2024

Guess, the best place to generate the schema definition would be as part of Upscheme because then we can leverage the Doctrine DBAL objects. I can imagine adding methods like toArray() or toJson() that can be used e.g. by a Laravel command to create the PHP files from the output. What do you think?

@MichelJonkman
Copy link
Author

Guess, the best place to generate the schema definition would be as part of Upscheme because then we can leverage the Doctrine DBAL objects. I can imagine adding methods like toArray() or toJson() that can be used e.g. by a Laravel command to create the PHP files from the output. What do you think?

Could you elaborate about what you mean with that?

@aimeos
Copy link
Owner

aimeos commented Oct 31, 2024

To generate schema migration files, you need a representation of the tables, column, views, etc. Currently, you use DBAL directly to get that representation but if you use Upscheme, it makes more sense to retrieve that information from Upscheme directly and in a representation which is better suited and easier to handle.

@MichelJonkman
Copy link
Author

Oh yes of course, I'll have to figure that all out when I go to work on it. I'm currently quite busy with other things but my own package will probably break due to updates soon so when I have some extra time I'll work on these things.

Thank you very much for making this package btw! It's a shame that this approach is not utilized more often, my feature request for Laravel also got very little traction... Is there an Aimeos Discord server I can join for better communication?

@aimeos
Copy link
Owner

aimeos commented Oct 31, 2024

Thank you very much for making this package btw! It's a shame that this approach is not utilized more often, my feature request for Laravel also got very little traction... Is there an Aimeos Discord server I can join for better communication?

Unfortunately not. Laravel maintainers seem to get very conservative and mainly want to use own ideas and code.

@MichelJonkman
Copy link
Author

Thank you very much for making this package btw! It's a shame that this approach is not utilized more often, my feature request for Laravel also got very little traction... Is there an Aimeos Discord server I can join for better communication?

Unfortunately not. Laravel maintainers seem to get very conservative and mainly want to use own ideas and code.

I have exactly the same experience, I had a lot of trouble trying to inject my custom migrator into the migrate command and the only response I got for asking about it is that I just shouldn't do it...

@aimeos
Copy link
Owner

aimeos commented Nov 4, 2024

Based on your feedback, we've implemented a DB::toArray() method to return an optimized representation of the schema objects and a generator for migrations. You can use it like this:

$config = [
	'db' => [
		'driver' => 'pdo_mysql',
		'host' => '127.0.0.1',
		'dbname' => '<database>',
		'user' => '<dbuser>',
		'password' => '<secret>'
	]
];

\Aimeos\Upscheme\Up::use( $config, 'migrations' )->create();

This will generate one file for each sequence, table and view in the passed
directory (./migrations/ in this example). If you have several databases and
want to create migrations for all of them at once, pass the connection keys
from the configuration to create():

$config = [
	'db' => [
		'driver' => 'pdo_mysql',
		// ...
	],
	'order' => [
		'driver' => 'pdo_oci',
		// ...
	]
];

\Aimeos\Upscheme\Up::use( $config, 'migrations' )->create( ['db', 'order'] );

@MichelJonkman
Copy link
Author

I had a quick look at the create() method and the basic usage of the package in general and have some feedback, when I have more free time I'll take a closer look at everything and can possibly create some pull requests for features that would be very nice to have for Laravel integration.

  1. The created migration files are extra verbose, like including charset and collation when those are the same as default, or not using the id() method. Of course it's unrealistic to want to generate a perfect migration file but I'm afraid that it might cause some migration files to have a lot of extra data to prune, which can lead to issues when a column has a different charset from the rest which could be accidentally removed with all the unneeded data.
  2. The code style of the created doesn't follow the PER (or PSR) coding style, since this is the largest and most well defined coding standard that PHP has I feel like it would be best to follow that at least for the generated files.
  3. The API deviates quite a lot from the Laravel API in some cases, though it's a personal preference when used outside of Laravel, when used inside a Laravel project it would be very nice if developers can keep using all the methods they're used to with only minor differences. When I work on Laravel integration I'll also be looking at how to improve this experience but we'll have to communicate on how since I don't want to create bloated classes full of aliases or create extra baggage that needs to be maintained but isn't used much.
  4. I'm not sure if I'm missing it but I don't see a good way of getting a list of changed tables before actually running the migrations. In the past I've had multiple issues with malformed migrations that would've caused data loss if not for the confirmation and diff of my own package, such a feature would greatly reduce such accidents in production environments.
  5. It would be nice if deleting a table would be as easy as removing the migration file from the directory, this would probably require a database table or file that keeps track of what tables were added using Upscheme. Using a table only to track which tables are managed by Upscheme is not plagued by the same issues as with normal migrations however. Another option would be to completely sync the migrations with the database, deleting any tables that don't exist in migrations, this would create issues when Upscheme is not used to manage the entire database for whatever reason so I would prefer the first solution to this.

While I haven't tested the package very well yet, so far all my issues were relatively minor like the previous feedback and I'm very happy with that! Not having the stress of having to figure out and manage the hardest part of the package is absolutely great so thank you very much for creating and maintaining this.

I asked earlier as well, but is there a Discord server or other social media platform where it's easier to discuss things in the future? If GitHub is preferred then that's okay but I feel like it will slow down communication a lot when it's only about small things or ideas.

@aimeos
Copy link
Owner

aimeos commented Nov 15, 2024

I had a quick look at the create() method and the basic usage of the package in general and have some feedback, when I have more free time I'll take a closer look at everything and can possibly create some pull requests for features that would be very nice to have for Laravel integration.

Sure, we appreciate any improvments :-)

  1. The created migration files are extra verbose, like including charset and collation when those are the same as default, or not using the id() method. Of course it's unrealistic to want to generate a perfect migration file but I'm afraid that it might cause some migration files to have a lot of extra data to prune, which can lead to issues when a column has a different charset from the rest which could be accidentally removed with all the unneeded data.

At the moment, the migrations are setting all non-empty values. It's subject to more investigations if this can be reduced. The id columns are a combination of several column properties, so the code generating the migrations will get more complicated then.

  1. The code style of the created doesn't follow the PER (or PSR) coding style, since this is the largest and most well defined coding standard that PHP has I feel like it would be best to follow that at least for the generated files.

Good point. It should be relatively easy to adapt that.

  1. The API deviates quite a lot from the Laravel API in some cases, though it's a personal preference when used outside of Laravel, when used inside a Laravel project it would be very nice if developers can keep using all the methods they're used to with only minor differences. When I work on Laravel integration I'll also be looking at how to improve this experience but we'll have to communicate on how since I don't want to create bloated classes full of aliases or create extra baggage that needs to be maintained but isn't used much.

There are some differences and you won't get a Laravel-like API without adding alias methods.

  1. I'm not sure if I'm missing it but I don't see a good way of getting a list of changed tables before actually running the migrations. In the past I've had multiple issues with malformed migrations that would've caused data loss if not for the confirmation and diff of my own package, such a feature would greatly reduce such accidents in production environments.

We don't think it's possible to get a list of changed tables but a dry-run option that outputs the executed statements would be no problem.

  1. It would be nice if deleting a table would be as easy as removing the migration file from the directory, this would probably require a database table or file that keeps track of what tables were added using Upscheme. Using a table only to track which tables are managed by Upscheme is not plagued by the same issues as with normal migrations however. Another option would be to completely sync the migrations with the database, deleting any tables that don't exist in migrations, this would create issues when Upscheme is not used to manage the entire database for whatever reason so I would prefer the first solution to this.

This is problematic for several reasons:

  • Upscheme should remain stateless (not using any tracing tables)
  • It can drop tables accidentally
  • We use it also for tables with mixed responsibilities (Laravel users table for example)

I asked earlier as well, but is there a Discord server or other social media platform where it's easier to discuss things in the future? If GitHub is preferred then that's okay but I feel like it will slow down communication a lot when it's only about small things or ideas.

Github issues are a great way for us to document decisions so we prefer them even if communication may be slightly slower :-)

@aimeos
Copy link
Owner

aimeos commented Nov 15, 2024

The generated migration files should now be PSR-2 compliant: a7641b0

@MichelJonkman
Copy link
Author

  1. I can possibly have a look at that sometime, the code structure seems quite simple which is great!
  2. Very nice! If I find any bits that still generate wrong I'll let you know.
  3. Are you open to adding aliases for this? Or maybe a separate class for the ColumnDefinition that can be injected? I don't know if adding aliases will lead to conflicts in the future, in my package I simply added most Laravel column methods similar to how it works in Upscheme but doing so here might cause conflicts with existing methods. If you're open to the idea then it should be fairly simple as I already have the code, though it uses the DBAL methods directly and not the Upscheme methods.
  4. I'll have to take a deeper look at the code but for my own package it was quite simple since DBAL can generate a diff object. Though it'll be a little different since Upscheme runs isolated tasks instead of doing it all at once, I think it can still be done however.
  5. That's understandable, it's not the biggest issue in a production environment but during development it can be quite nice. I'll see how much of an issue it is for me as it is right now and I can always write a separate package for it.

I don't have much experience with managing packages so I have quite a lot to learn but apart from a Laravel integration I'm also thinking of creating a Symfony/Console version with batteries included, this would mostly be for myself but maybe it could also help drive adoption for the package.

@aimeos
Copy link
Owner

aimeos commented Nov 16, 2024

  1. Are you open to adding aliases for this? Or maybe a separate class for the ColumnDefinition that can be injected? I don't know if adding aliases will lead to conflicts in the future, in my package I simply added most Laravel column methods similar to how it works in Upscheme but doing so here might cause conflicts with existing methods. If you're open to the idea then it should be fairly simple as I already have the code, though it uses the DBAL methods directly and not the Upscheme methods.

There might be a better way as Upscheme supports macros like Laravel does. You can simple add methods to the existing table or column class using:

\Aimeos\Upscheme\Schema\Column::macro('nullable', function($val) {
    return $this->null(true);
});

I don't have much experience with managing packages so I have quite a lot to learn but apart from a Laravel integration I'm also thinking of creating a Symfony/Console version with batteries included, this would mostly be for myself but maybe it could also help drive adoption for the package.

More integrations into existing frameworks would be great! :-)

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

No branches or pull requests

2 participants