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

base.sql should be optional. #8

Merged
merged 5 commits into from
Mar 19, 2018
Merged

base.sql should be optional. #8

merged 5 commits into from
Mar 19, 2018

Conversation

Naouak
Copy link
Contributor

@Naouak Naouak commented Mar 15, 2018

When you start from scratch and don't have any database, the first migration contains all needed table creations and base.sql is empty. An empty base.sql generates an "Empty Query" error.

This merge request makes base.sql optional instead of mandatory.

@Naouak Naouak changed the base branch from master to 2.0.4 March 15, 2018 11:06
@byjg
Copy link
Owner

byjg commented Mar 15, 2018

Hi!

First of all, thank you again!!!

Specifically about this PR why instead to make it just optional, why we do it explicit optional by passing a parameter for that?

For example:

The Migration's constructor may to have this sintax:

public function __construct(UriInterface $uri, $folder, $requiredBase = true)

and instead to simply remove it, you will check it before:

if ($requiredBase && !file_exists($this->folder . '/base.sql')) {
}

So, if you accept my suggestion, you'll have to change 3 files:

  • src/Migration.php (by addind the code above)
  • tests/MigrationTest.php (by adding a test for validate the optional base.sql)
  • src/Console/ConsoleCommand.php (by adding the parameter --no-base)

Why base.sql is required?

Even you starting a database from scratch it is important you have a reference with the most first script that started the database.

Sometimes the number of revisions is so big and take so many time to generate a complete build, that you decide to generate a new base from an specific version and the system will start the migrations UP and DOWN from that version (since the base.sql already have the migration table);

So, the base is the first script for RESET and the reference for your CI environment.

I hope you agreed with me.

@Naouak
Copy link
Contributor Author

Naouak commented Mar 19, 2018

Done.

@byjg byjg merged commit be3b9a3 into byjg:2.0.4 Mar 19, 2018
@byjg
Copy link
Owner

byjg commented Mar 19, 2018

Great!!!!!!!

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.

2 participants