Cast to integer to avoid Spanner GCP incompatibility

[gabrielfs7]

Summary

Q	A
Type	bug
BC Break	no
Fixed issues	Avoid delegate to DB responsibility to cast execution_time and fix GCP Spanner issue

I am currently receiving error from GCP when trying to persist data using Spanner:

An exception occurred while executing 'INSERT INTO doctrine_migration_versions (version, executed_at, execution_time) VALUES (?, ?, ?)' with params ["oat\\generis\\migrations\\Version202009301828472348_generis", "2020-10-02T11:54:13.671062Z", 130]:  
                                                                                                                                                                                                                                                            
  {                                                                                                                                                                                                                                                         
      "message": "Invalid value for bind parameter param3: Expected INT64.",                                                                                                                                                                                
      "code": 3,                                                                                                                                                                                                                                            
      "status": "INVALID_ARGUMENT",                                                                                                                                                                                                                         
      "details": [                                                                                                                                                                                                                                          
          {                                                                                                                                                                                                                                                 
              "@type": "grpc-server-stats-bin",                                                                                                                                                                                                             
              "data": "<Unknown Binary Data>"                                                                                                                                                                                                               
          }                                                                                                                                                                                                                                                 
      ]                                                                                                                                                                                                                                                     
  }        

Cause the 130 is actually 130.0 (a float).

Since migrations always expects an integer here I would like to cast this value to avoid driver incompatibilities

The issue was reported here: #1061

[mike-ionut-mihai-sandu-tao]

looks good from my side,
and i would go for this because the intent is declared below on the Types::INTEGER
at tline https://github.com/doctrine/migrations/pull/1060/files#diff-fd5b2ea887e9abe3b606201b76d10bb9R144

[xvzf]

Nice!

[goetas]

To me looks good and reasonable. Is there a way to add a test for such change?

[gabrielfs7]

To me looks good and reasonable. Is there a way to add a test for such change?

@goetas Seems this test was already done before, but with SQLite driver it always passed :)

https://github.com/doctrine/migrations/blob/3.0.x/tests/Doctrine/Migrations/Tests/Metadata/Storage/TableMetadataStorageTest.php#L161

[goetas]

@greg0ire do you have suggestions here? (merge it as it is?)

[SenseException]

If an execution time appears that is bigger than int can handle, then there is a much bigger problem that is not related to doctrine/migrations. So I guess this change should be fine.

I recommend to add a unit test because this change has to be handled to prevent breaks in the future in case this would be a major problem for users.

BTW: Should this PR target 3.0.x? Is this a bugfix?

[stof]

this is indeed a bugfix

[gabrielfs7]

If an execution time appears that is bigger than int can handle, then there is a much bigger problem that is not related to doctrine/migrations. So I guess this change should be fine.

I recommend to add a unit test because this change has to be handled to prevent breaks in the future in case this would be a major problem for users.

BTW: Should this PR target 3.0.x? Is this a bugfix?

I will check better way to test this and come back soon. Thanks all for the input.

[gabrielfs7]

Tests added @goetas @SenseException @stof Can you please take a look and merge if is ok. Thank you in advance

[greg0ire]

Please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
$userName@users.noreply.github.com, that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

Also, please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

1. git rebase -i origin/master, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
2. A window will show up with many lines, replace pick with fixup on every line but the first one
3. Close your editor, git should do its magic, and you should end up with one commit
4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

And finally, since this is a bugfix, I think you should target 3.0.x

[greg0ire]

Let me help you with the commits :P

[gabrielfs7]

Let me help you with the commits :P

I appreciate, thanks! :)

[goetas]

In my experience mocks have been always a problem. The less the better. I've created #1063 that solves the same problem just with a different testing strategy.

Can you please review that version ? (@greg0ire @gabrielfs7 )

[gabrielfs7]

#1063

@goetas I have dropped a comment there: https://github.com/doctrine/migrations/pull/1063/files#r507585488

Unfortunately the test still passes if you remove the cast. The way I did the test breaks if the cast is removed.

[greg0ire]

Thanks @gabrielfs7 !