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

Replace save() with update() #735

Closed
wants to merge 1 commit into from
Closed

Conversation

davidspeijer
Copy link

Update() should be used instead of save().

@LordSimal
Copy link
Contributor

->save() is the recommended method to use since it automatically calls either $this->update(); or $this->create(); depending on if a table needs to be created or updated.

or is there a specific reason why you think this is better?

@davidspeijer
Copy link
Author

For deleting columns the ->save() wasn't working for the migration I created. When I recplaced it with ->update() it did. When I bake a migration for deletion it also uses ->update().

@LordSimal
Copy link
Contributor

LordSimal commented Aug 8, 2024

What does "was not working for me" mean? Did you get an error? Did it just apply the migration but nothing happened?

I just tested it with latest CakePHP 5 and it works fine with ->save();

@davidspeijer
Copy link
Author

davidspeijer commented Aug 8, 2024

When i tried it in a cakephp 4.5.6 project it did not remove the field.

My code;

public function up(): void
{
    $table = $this->table('table');
    $table->removeColumn('field');
    $table->save();
}

@LordSimal
Copy link
Contributor

I have an example app with exactly that version and your code works fine for me.

Are you certain you have the correct datasource set and looking at the correct database?

@davidspeijer
Copy link
Author

Yes, I am. In the same migration I could update fieldnames. But the removal didn't work. Maybe there was an other error. If you are sure the code from the book is correct, you can ignore this commit.

@LordSimal
Copy link
Contributor

can you post your whole example? maybe there is a problem if multiple operations are queued up.

@davidspeijer
Copy link
Author

davidspeijer commented Aug 8, 2024

I will remove my code later. I think my code was like this (altered it until it worked for me).

<?php

declare(strict_types=1);

use Migrations\AbstractMigration;

class Migration01 extends AbstractMigration
{

    public function up(): void
    {
        $table = $this->table('table');
        $table->removeColumn('field');
        $table->save();
    }

    /**
     * Change Method.
     *
     * More information on this method is available here:
     * https://book.cakephp.org/phinx/0/en/migrations.html#the-change-method
     *
     * @return void
     */
    public function change(): void
    {
        $table = $this->table('table');

        $table
            ->renameColumn('xx', 'yy');
       
        $table->update();
    }
}

@LordSimal
Copy link
Contributor

i am pretty sure you can't mix change() and up() methods 😅

either do only change() or do both up() and down() respectivly

@davidspeijer
Copy link
Author

Thanks for the reply. Maybe we can add it to the docs not to do that 😄

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

Successfully merging this pull request may close these issues.

2 participants