cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.46k stars 889 forks source link

No More migration possible : error creating AbstractMigration because of type int for the second parameter #2111

Closed zeearth closed 1 year ago

zeearth commented 2 years ago

Hello,

Since upgrade to 0.13.0, we can't anymore run migration scripte cause of the following error : TypeError: Argument 2 passed to Phinx\Migration\AbstractMigration::__construct() must be of the type int, string given,

Error throwed by line 824 into Phinx\Migration\Manager.php // instantiate it $migration = new $class($environment, $version, $this->getInput(), $this->getOutput());

$version is initialised at line 779 by the method Util::getVersionFromFileName which return a string value

src/Phinx/Migration/AbstractMigration.php image

dereuromark commented 2 years ago

It seems those are from this commit https://github.com/cakephp/phinx/commit/88bc125088bfea25ab19c8c5b6f50078dcf26e11#diff-72fcd2be7d86c10f591624e69dcb3bdc646980a23ecf987159a13bb399e7d6ebR73

We might want to then use string instead here, as per other recent changes.

MasterOdin commented 2 years ago

The two paths would be:

  1. Make it so that $version can be a string everywhere
  2. Make it so that $version is an integer everywhere (what we're currently doing)

For case (2), it would just be modifying Util::getVersionFromFileName so that it casts its regex match to an integer and update the type appropriately.

The root cause of the bug is that while we added typing everywhere, we didn't add declare(strict_types=1); everywhere as well so there's some amount of implicit conversion happening that need to track down and eliminate (such as here, as well as in casting $version in the CLI commands appropriately).

dereuromark commented 1 year ago

Should we proceed here? This also blocks https://github.com/cakephp/migrations/pull/517 now, as it is unclear where to go

As for string change, this would probably be another major now.

zeearth commented 1 year ago

Still not working when upgrade to phinx 0.13.3... I have got error message from duplicate version. Trying to pass the --date= option to my call, but stil have a cast of string to int in the function getVersionFromFileName in file vendor/robmorgan/phinx/src/Phinx/Util/Util.php @dereuromark

MasterOdin commented 1 year ago

@zeearth Can you provide a link to a repo that reproduces the bug?

dereuromark commented 1 year ago

We did not do any release yet If you want to test it, you need to use 0.x-dev as constraint for now.

MasterOdin commented 1 year ago

This should have been fixed in 0.13.3 from #2150. There were no changes in 0.13.4 related to this codepath, so I'd expect the same error on 0.x-dev as 0.13.3.

dereuromark commented 1 year ago

Ah, you are right Then we need to know more about how this can be reproduced or the setup in question. Right now we dont know where to look for.

zeearth commented 1 year ago

Do you need me to retest and put the errors here?

zeearth commented 1 year ago

image

My migration file : 20230106220000_TestForGitHub.php

image

A dump from my console :

C:\dev\neard\bin\php\current\php.exe C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\bin\phinx migrate -c phinxMysql.yml
Phinx by CakePHP - https://phinx.org.

using config file phinxMysql.yml
using config parser yml
using migration paths 
 - C:\dev\src\mpec\NextGen\api-nextgen\data\databases\migrationsMysql\mpec
using seed paths 
 - C:\dev\src\mpec\NextGen\api-nextgen
warning no environment specified, defaulting to: local
using adapter mysql
using database mpec
ordering by creation time
InvalidArgumentException: Duplicate migration - "./data/databases/migrationsMysql/mpec\20190101000002_Applications.php" has the same version as "2147483647" in C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\src\Phinx\Migration\Manager.php:782
Stack trace:
#0 C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\src\Phinx\Migration\Manager.php(313): Phinx\Migration\Manager->getMigrations('local')

The phinxlog table for the mentionned file into the trace : image

I had a var_dump in the impacted file :

string(108) "C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\src\Phinx\Util\Util.php -- getVersionFromFileName"
array(3) {
  ["fileName"]=>
  string(25) "20190101000001_Params.php"
  ["martches['0']"]=>
  string(14) "20190101000001"
  ["int"]=>
  int(2147483647)
}
string(131) "Call from C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\src\Phinx\Migration\Manager.php in 'getMigrations' at line 779"
string(108) "C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\src\Phinx\Util\Util.php -- getVersionFromFileName"
array(3) {
  ["fileName"]=>
  string(31) "20190101000002_Applications.php"
  ["martches['0']"]=>
  string(14) "20190101000002"
  ["int"]=>
  int(2147483647)
}
string(131) "Call from C:\dev\src\mpec\NextGen\api-nextgen\vendor\robmorgan\phinx\src\Phinx\Migration\Manager.php in 'getMigrations' at line 779"

So we have two different migration file who gives same version parsed in int. Is that enough for you or do you need more informations ?

zeearth commented 1 year ago

Same error with 0.x-dev InvalidArgumentException: Duplicate migration - "./data/databases/migrationsMysql/mpec\20190101000002_Applications.php" has the same version as "2147483647"

dereuromark commented 1 year ago

You seem to be using an outdated version ("martches" was long fixed) Never use * as constraint. Use ^0.13.3 here.

zeearth commented 1 year ago

Same error image

image

dereuromark commented 1 year ago

Can you post again the string value that should be the int value? Looking into the code it should be nicely cast, since it is a real integer value (e.g. 20190101000002)

"martches['0']"]=> string(14) "20190101000002" ["int"]=> int(2147483647)

Is it possible you are not using a 64bit system and there the max int is lower than this timestamp int? on my system PHP_INT_MAX is 9223372036854775807 > 20190101000002 On a 32 bit it would be 2,147,483,647 < 20,190,101,000,002

dereuromark commented 1 year ago

@MasterOdin We might want to add to the docs that 64bit is a hard requirement since 0.13

@zeearth Using WIN directly is never a good idea, especially not for PHP Best to use a container, e.g. docker, like devilbox or alike Those provide a safe system that is out of the box working and also closer to your prod system then.

zeearth commented 1 year ago

seems to be the point 👍 image

We are using neard. Getting to find a solution. So the problem is not phinx. So you can close this issue, may be add 64bits as required in the documentation.

MasterOdin commented 1 year ago

Glad we got this figured out. PHP does have 64-bit builds for 7+ available on https://windows.php.net/ and so this may be a bug with neard that it's installing a 32-bit PHP, unless you're using a 32-bit version of Windows, though that shouldn't be exactly common anymore...

pabloelcolombiano commented 1 year ago

Hi,

we are having some troubles with customers using 32 bit version of Raspbian/Debian.

  1. What was the motivation to move from string to int in the first place? Since CakePHP highly recommends to use migrations, the 64 bit requirement indirectly also restrains Cake to it.

  2. Would you be interested in a PR to switch back to the version being a string?