wintercms / wn-builder-plugin

GUI for building plugins in Winter CMS
MIT License
34 stars 12 forks source link

Newly created fields on the model - not correctly increase number of class name migration file #51

Open dimti opened 1 year ago

dimti commented 1 year ago

Strange behaviour - if i created new field in database - on first this actions on the table - that is okay. But next field - have 2 number - as suffix of migration class name and colissing with previous migration class name

image

dimti commented 1 year ago

Number inscreases in that code block: classes/MigrationModel.php::makeScriptFileNameUnique

And this use snake-case separator between base file name and suffix number:

    public function makeScriptFileNameUnique()
    {
        $updatesPath = $this->getPluginUpdatesPath();
        $baseFileName = $fileName = $this->scriptFileName;

        $counter = 2;
        while (File::isFile($updatesPath.'/'.$fileName.'.php')) {
            $fileName = $baseFileName.'_'.$counter;
            $counter++;
        }

        return $this->scriptFileName = $fileName;
    }

But in currently class and in method assignFileName used Str::snake helper for assigned file name for migration - and that uses from migrations part of wn-builder.

In Migration Model save method - strangelly activate reassign file name for existing migration. I turn off this behaviour

From:

        if (!strlen($this->scriptFileName) || !$this->isNewModel()) {
            $this->assignFileName();
        }

To:

        if (!strlen($this->scriptFileName) || $this->isNewModel()) {
            $this->assignFileName();
        }

if that is acceptable solution - please give me call backout about pr.

https://github.com/dimti/wn-builder-plugin/commit/b1c4e3fa30b7165f78f7122fe45821f3997000d0

LukeTowers commented 1 year ago

Personally I'd like to see us switch to using the anonymous class migration method that we now use in the core scaffolds: https://github.com/wintercms/winter/blob/develop/modules/system/console/scaffold/migration/migration.create.stub; although that would necessitate a new major version to indicate that we only support Laravel 9 / Winter v1.2 going forward.

SledgehammerPL commented 1 year ago

I verified this patch, and works as expected... Great development!