Found a problem after release version 1.11.0 of propel-orm

giovanni.piemontese

Hi Guys
I found today a problem after release version 1.11.0 of propel-orm https://github.com/spryker/propel-orm/releases

The version tag 1.11.0 required the propel version alpha10. This version has a bug in propel migrate console command https://github.com/propelorm/Propel2/compare/2.0.0-alpha8...2.0.0-alpha10#diff-1ccdce29404b8984d1b2101f0069a564
In MigrationMigrateCommand they return now CODE_ERROR instead false at line 84 but for symfony command vendor/symfony/console/Command/Command.php:262 false was a success and now the command is aborted.

@valerii.trots can u please check internal how we can fix this? Normally propel have to be fixed. But Spryker has offical release propel-orm version 1.11 that required propel alpha 10 version

The problem now is that our install and deployment scripts just fail due the exit code of migration command console even if no migrations were executed.

Has anyone from today the same problem? One possible solution is of course to constraint the spryker/propel-orm version to 1.10 in composer.json.

Valerii Trots

According to our development, you need to adjust the code on your side:

The proper error code here is returned (as per Symfony specs), as such their code should be adjusted rather.

Symfony public function run(InputInterface $input, OutputInterface $output)
@return int The command exit code
is the contract. And will in the future be enforced, as such the change is needed.

<https://github.com/spryker/propel-orm/releases/tag/1.11.0> will clarify this?

giovanni.piemontese

Yes of course.. but the return code is wrong...

In the case there were not migration to be executed then propel alpha8 just return false

Now alpha10 has to be compatible with symfony 5 that enforce to return int. OK. Clear..

But the return of symfony command is the follwowing:

return is_numeric($statusCode) ? (int) $statusCode : 0; 

and as u see false means (for backward compatibility) SUCCESS

Now Migration Command Console in case no migration needed return 1 instead falseand this for symfony command is ERROR and not more SUCCESS

Propel alpha10 has a bug. They just replace the return false with return CODE_ERROR but this is not correct in every case, aboveall in the migraiton command console in the case no migration needed.. Especially here:

if (!$manager->getFirstUpMigrationTimestamp()) {
            $output->writeln('All migrations were already executed - nothing to migrate.');

            return static::CODE_ERROR;
        }

before here was return false!

@valerii.trots if u want i can discuss about it with the developer that said that i need to adjust the code on my side

Valerii Trots

No worries, let's play ping pong with them. 🙂

giovanni.piemontese

🙂 ok.. just investigate in details the changes
alpha8...alpha10 and together with \Symfony\Component\Console\Command\Command::run return statement

giovanni.piemontese

some news?

Valerii Trots

<https://github.com/propelorm/Propel2/pull/1595/files#diff-1ccdce29404b8984d1b2101f0069a564R84> is the change, and return false was supposed to make a 1 exit code, so changing to 1 directly is actually the correct change.

The fact that this actually should have returned true (and thus now 0) is a different topic. He might be right, and this should be fixed.
But how than this be a regression if it was always wrong so far?


I opened a PR for our AA to discuss: <https://github.com/propelorm/Propel2/pull/1617>

Valerii Trots

You can forward this to the project, they can +1 it if thats correct
and hopefully we can include this in the next alpha release.

giovanni.piemontese

ok, danke!