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

binaryuuid wrongly seen as binary in tests #17420

Closed
dereuromark opened this issue Nov 9, 2023 · 8 comments · Fixed by cakephp/phinx#2239
Closed

binaryuuid wrongly seen as binary in tests #17420

dereuromark opened this issue Nov 9, 2023 · 8 comments · Fixed by cakephp/phinx#2239
Assignees
Labels
Milestone

Comments

@dereuromark
Copy link
Member

dereuromark commented Nov 9, 2023

Description

When opening the browser, all seems well for a uuid table defined in migrations as

->addColumn('uuid', 'binaryuuid', [
		'default' => null,
		'limit' => null,
		'null' => false,
	])

and correctly in DB as binary(16)

When running tests however, on find() the resource is written directly into the entity field as such (not as converted strings), making the tests fail.

object(Sandbox\Model\Entity\ExposedUser) id:0 {
  'id' => (int) 1
  'name' => 'Foo Bar'
  'uuid' => (resource (stream)) Resource id cakephp/cakephp#2823

The reason seems to be that the type of that column is suddenly binary only, in tests.
So the BinaryUuidType::toPhp() is never invoked, of course.

dd(TableRegistry::getTableLocator()->get('Sandbox.ExposedUsers')->getSchema()->getColumn('uuid'));

[
  'type' => 'binary',
  'length' => null,
  'null' => false,
  'default' => null,
  'precision' => null,
  'comment' => null
]

This seems to be a regression coming from 4.x where this worked (and tests were green)

Note: This seems to happen with any type of DB, Mysql/Sqlite...
Refs https://github.com/dereuromark/cakephp-sandbox/actions/runs/6806484697/job/18507784939#step:10:384

The only difference is that in 4.x I used the fixture structure directly, with 5.x I was now forced to move to Migrator run.

CakePHP Version

5.x

PHP Version

8.2

@markstory
Copy link
Member

Is the schema being generated by your migration different than the fixture schema?

@dereuromark
Copy link
Member Author

No
https://github.com/dereuromark/cakephp-sandbox/blob/4.x/plugins/Sandbox/tests/Fixture/ExposedUsersFixture.php#L21
and now only

->addColumn('uuid', 'binaryuuid', [
				'default' => null,
				'limit' => null,
				'null' => false,
			])

for both actual DB and test setup.

Is it possible to see the results? As the DB is resetting after each test right now.

@dereuromark
Copy link
Member Author

dereuromark commented Nov 9, 2023

This branch/PR showcases it in real life example: dereuromark/cakephp-sandbox#66
Only failing issues remaining is this regression on the binaryuuid type (compared to green 4.x).

Also:
bin/cake bake migration_snapshot produces the correct 'binaryuuid' type, so the issue must be specific to the test harness around running the migrator.

@markstory
Copy link
Member

markstory commented Nov 9, 2023

Is it possible to see the results? As the DB is resetting after each test right now.

You could dump the generated schema out of the database server.

I'm guessing that the column types we're generating aren't being picked up by schema reflection as binaryuuid.

@markstory markstory self-assigned this Nov 10, 2023
@markstory
Copy link
Member

Looking at the schema your currently generating through migrations I see

CREATE TABLE `exposed_users` (`id` INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, `name` VARCHAR(100) NOT NULL, `uuid` BINARY_BLOB NOT NULL, `created` DATETIME_TEXT NOT NULL, `modified` DATETIME_TEXT NOT NULL);

Which does not qualify as binaryuuid because the length is wrong. It looks like phinx/migrations is currently generating BINARY_BLOB for the binaryuuid type which doesn't seem correct.

The root cause of this issue is that phinx currently maps binaryuuid to the BINARY_BLOB type which will never be counted as binaryuuid by cakephp because we only accept binary(16).

If we cannot make this change in phinx because of backwards compatibility, then this is more fuel under the fire for cakephp/migrations#647 so that we can make the change in migrations.

I know you're not a fan of having to use migrations for test schema. Would having the abstract format that we use for cakephp exposed as a public interface be a better solution for you?

@dereuromark
Copy link
Member Author

Sounds like a bugfix for phinx, since this doesn't seem to be fully working.

@dereuromark
Copy link
Member Author

Or we shim it in migrations plugin

@dereuromark dereuromark transferred this issue from cakephp/cakephp Nov 12, 2023
@dereuromark
Copy link
Member Author

Yeah, so turns out that
cakephp/phinx#1734
was the problem all along, it probably should be a different type than this.

But cakephp/phinx#2239 doesnt quite fix it either yet

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 a pull request may close this issue.

2 participants