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

[10.x] Improvements for artisan migrate --pretend command πŸš€ #48768

Merged
merged 12 commits into from
Oct 26, 2023

Conversation

NickSdot
Copy link
Contributor

@NickSdot NickSdot commented Oct 18, 2023

With this PR, I propose to add value to the php artisan migrate --pretend command by

  1. adding DB::ignorePretendModeForCallback(Closure $callback) to allow us to execute a given database query closure in migrations, while in pretend mode.
  2. populating the output with the actual data it will run on.

Benefits

In the same spirit as my recently accepted PR this PR seeks to allow us to see as clearly as possible what we're dealing with, instead of doing guess work.

  • every single statement that would be run will also be displayed in pretend mode
  • the statements will be populated with their actual data instead of placeholders

Usage

public function up(): void
{
    $foo1 = DB::table('customer_sites')->get();
    
    dump($foo1); // empty collection

    $foo2 = DB::ignorePretendModeForCallback(function () {
       // pretend mode deactivated
        return DB::table('customer_sites')->get();
       // pretend mode reactivated
    });
    
    dump($foo2); // populated collection we can work with
}

Status Quo

Problem 1

When running php artisan migrate --pretend no database query will be executed. As the mode says, they are pretended. This also means that statements that depend on a previous query to the database will not be shown in the output. I just do not know what I am really dealing with because only a part of the actual statements is visible to me. Here is a real life example:

Click to expand code

Note: Code simplified

public function up(): void
{   
    $tables = DB::select('SHOW TABLES');
    
    foreach ($tables as $table) {


       // every statement from here on will not be visible in the command pretend output. 
       // it does not matter whether we use foreach or DB::table()->get()->each()


        $tableName = $table->{'Tables_in_' . config('database.connections.mysql.database')};
        
        $columns = DB::select("SHOW COLUMNS FROM $tableName");
        
        foreach ($columns as $column) {

            if ($column->Type == 'timestamp' || $column->Type == 'datetime') {
                DB::statement("UPDATE $tableName SET $column->Field = '1988-01-01 00:00:00' WHERE $column->Field = '0000-00-00 00:00:00';");
            }

            if (($column->Type == 'timestamp' || $column->Type == 'datetime') && $column->Default == '0000-00-00 00:00:00') {
                DB::statement("ALTER TABLE $tableName MODIFY COLUMN $column->Field datetime NULL DEFAULT NULL");
            }

            if ($column->Type == 'timestamp' || $column->Type == 'datetime') {
                DB::statement("UPDATE $tableName SET $column->Field = NULL WHERE $column->Field = '1988-01-01 00:00:00';");
            }

        }
  }


   // this is visible


    DB::statement("ALTER TABLE basket_features MODIFY domain VARCHAR(191);");
    DB::statement("UPDATE contact_requests SET name = LEFT(name, 100) WHERE CHAR_LENGTH(name) > 191;");
    DB::statement("UPDATE emaillogs SET to_name = LEFT(to_name, 100) WHERE CHAR_LENGTH(to_name) > 191;");
    DB::statement("UPDATE emaillogs SET from_name = LEFT(from_name, 100) WHERE CHAR_LENGTH(from_name) > 191;");

    foreach ($tables as $table) {


       // every statement from here on will not be visible in the command pretend output. 
       // it does not matter whether we use foreach or DB::table()->get()->each()

        DB::statement("ALTER TABLE $tableName CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci");
    }

}
Click to expand output

Note: Output simplified, and already using real values instead of placeholders

  // current
  ⇂ SHOW TABLES  
  ⇂ ALTER TABLE basket_features MODIFY domain VARCHAR(191);  
  ⇂ UPDATE contact_requests SET name = LEFT(name, 100) WHERE CHAR_LENGTH(name) > 191;  
  ⇂ UPDATE emaillogs SET to_name = LEFT(to_name, 100) WHERE CHAR_LENGTH(to_name) > 191;  
  ⇂ UPDATE emaillogs SET from_name = LEFT(from_name, 100) WHERE CHAR_LENGTH(from_name) > 191; 

The current output in my specific scenario is missing >170 statements like these:

  // missing
  ⇂ SHOW COLUMNS FROM emaillogs  
  ⇂ UPDATE emaillogs SET created_at = '1988-01-01 00:00:00' WHERE created_at = '0000-00-00 00:00:00'
  ⇂ UPDATE emaillogs SET updated_at = '1988-01-01 00:00:00' WHERE updated_at = '0000-00-00 00:00:00'
  ⇂ ALTER TABLE emaillogs MODIFY COLUMN created_at datetime NULL DEFAULT NULL
  ⇂ ALTER TABLE emaillogs MODIFY COLUMN updated_at datetime NULL DEFAULT NULL
  ⇂ UPDATE emaillogs SET created_at = NULL WHERE created_at = '1988-01-01 00:00:00'
  ⇂ UPDATE emaillogs SET updated_at = NULL WHERE updated_at = '1988-01-01 00:00:00'
  ⇂ ALTER TABLE emaillogs CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci

Only with the full context I am able to decide whether I considered all edge cases for every table and column. This, currently, is not possible.

Problem 2

The current output of the migrate pretend command is something like this:
insert into "blogs" ("url") values (?), (?) .

This does not help me to confirm that the data I expect really will be used when running the command in "hot mode". What I expect is to see the exact query that will be run:
insert into "blogs" ("url") values ('www.janedoe.com'), ('www.johndoe.com')

Since @tpetry already did the hard work (πŸš€β€οΈ) in #47507 we should leverage this functionality in the pretend command as well.

When merged

Before

public function up(): void
{
    // other schema stuff for 'blogs' table here

    DB::table('people')->get()->each(function ($person, $key) {
        DB::table('blogs')->where('blog_id', '=', $person->blog_id)->insert([
            'id' => $key+1,
            'name' => "{$person->name} Blog",
        ]);
    });
}
// Console Output

⇂ create table "blogs" ("id" integer primary key autoincrement not null, "url" varchar, "name" varchar)
⇂ insert into "blogs" ("url") values (?), (?) 
⇂ ALTER TABLE 'pseudo_table_name' MODIFY 'column_name' VARCHAR(191) 
⇂ select * from "people"  

After

public function up(): void
{
    // other schema stuff for 'blogs' table here

    $tablesList = DB::ignorePretendModeForCallback(function () {
        return DB::table('people')->get();
    });

    $tablesList->each(function ($person, $key) {
        DB::table('blogs')->where('blog_id', '=', $person->blog_id)->insert([
            'id' => $key+1,
            'name' => "{$person->name} Blog",
        ]);
    });
}
// Console Output

⇂ create table "blogs" ("id" integer primary key autoincrement not null, "url" varchar, "name" varchar) 
⇂ insert into "blogs" ("url") values ('www.janedoe.com'), ('www.johndoe.com') // actual data visible
⇂ ALTER TABLE 'pseudo_table_name' MODIFY 'column_name' VARCHAR(191) 
⇂ select * from "people" 
⇂ insert into "blogs" ("id", "name") values (1, 'Jane Doe Blog') // before missing
⇂ insert into "blogs" ("id", "name") values (2, 'John Doe Blog') // before missing

Considerations

1. Bindings
In DB::logQuery() which is responsible for creating the output, I decided to substitute the bindings only while in pretend mode. I would appreciate feedback here.

$query = $this->pretending === true
     ? $this->queryGrammar?->substituteBindingsIntoRawSql($query, $bindings) ?? $query
      : $query;

2. Misuse
Admittedly, people can misuse this and break things. When I started to work on this PR, I tried to allow only read queries. Got complicated, and I skipped. People can also break things with stevebauman/unfinalize. I think that is acceptable and everyone should be able to decide for themselves. No nanny needed, though.

3. Naming
I first named the method DB::doNotPretend(). It's shorter, but I feel like DB::ignorePretendModeForCallback() is more clear.

4. BC break
Not sure if the binding change in whatever way could be a BC break for people. If that is the case, I would propose to add a config option in 10.x and make it default in 11.

@tpetry
Copy link
Contributor

tpetry commented Oct 18, 2023

This is kind of complicated to solve a specific edge case. Would using another connection work for executing the not pretended queries? I havenβ€˜t checked yet whether all connections are set to pretended mode or only the one for the migration.

But I am all for solving problem 2! Youβ€˜re correct that the real query should be shown.

@NickSdot
Copy link
Contributor Author

NickSdot commented Oct 18, 2023

This is kind of complicated to solve a specific edge case. Would using another connection work for executing the not pretended queries? I havenβ€˜t checked yet whether all connections are set to pretended mode or only the one for the migration.
But I am all for solving problem 2! Youβ€˜re correct that the real query should be shown.

You mean like this? Yes.

public function up(): void
{
    $foo1 = DB::table('SOME_TABLE')->get();

    $firstConnection = DB::ignorePretendModeForCallback(function (){
        return DB::connection('mysql')->table('SOME_TABLE')->get();
    });

    $otherConnection = DB::ignorePretendModeForCallback(function (){
        return DB::connection('mysql2')->table('OTHER_TABLE')->get();
    });


    dump($foo1); // empty collection
    dump($firstConnection); // populated collection we can work with
    dump($otherConnection); // populated collection we can work with
}

Edit 1:
Works of course also without defining the connection for the default connection in the callback.

Edit 2:
When the property protected $connection is set in the migration itself, then DB::connection('connection_name') on the facade that will be passed to DB::ignorePretendModeForCallback() also must be set.

However, this is the exact same behaviour as already known from other contexts (e.g. laravel/docs#9059 and #36596). The pretend flag is, in fact, even ignored and statements will be executed if the connection is not the same as set in in the migration property. One should, in general, just not attempt to manipulate or create stuff for different connections in one migration. One migration writes to one connection.

However, pulling data in from another connection works just fine. Here is an example:

return new class extends Migration {

--> protected $connection = 'mysql2';

    public function up(): void
    {
-->     Schema::connection('mysql2')->create('people', function (Blueprint $table)
        {
            $table->increments('id');
            $table->string('name');
            $table->timestamp('created_at')->nullable();
        });

        $otherConnection = DB::ignorePretendModeForCallback(function () {
-->          return DB::connection('mysql')->table('websites')->get()->toArray();
        });

        foreach ($otherConnection as $item) {
            
            // create stuff on the connection defined in this migration
            // with data from another connection.

-->       Schema::connection('mysql2')->create("websites_{$item->id}_backup", function (Blueprint $table) use ($item)
            {
                $table->increments('id');
                $table->string('name');
                $table->string('ref')->default($item->id);
                $table->timestamp('created_at')->nullable();
            });
        }
}

// output
//  ⇂ create table `people` (`id` int unsigned not null auto_increment primary key, `name` varchar(255) not null, `created_at` timestamp null) default character set utf8mb4 collate 'utf8mb4_unicode_ci'  
//  ⇂ create table `websites_12_backup` (`id` int unsigned not null auto_increment primary key, `name` varchar(255) not null, `ref` varchar(255) not null default '12', `created_at` timestamp null) default character set utf8mb4 collate 'utf8mb4_unicode_ci'  

Not sure if things like this should be tested with additional tests here. Personally, I tend to say it is out of scope of this PR and feature.

Edit 3:
Lastly, to answer this:

whether all connections are set to pretended mode or only the one for the migration.

Interestingly, you don't need to wrap a query to a different connection into DB::ignorePretendModeForCallback(). It also works without. This means, pretend mode is set for the connection of the migration connection – not for all. Not really related to this PR, but this is surprising me somewhat. If pretending would be set for all connections, I think, it would likely solve all the weird issues, like the ones I mentioned above.

@NickSdot NickSdot changed the title [10.x] Improvements for artisan migrate --prepend command πŸš€ [10.x] Improvements for artisan migrate --pretend command πŸš€ Oct 18, 2023
@taylorotwell taylorotwell merged commit 4e43ca0 into laravel:10.x Oct 26, 2023
20 checks passed
@NickSdot NickSdot deleted the pretend branch October 26, 2023 18:09
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

Successfully merging this pull request may close these issues.

None yet

3 participants