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

[Messenger] schema assets filter causes TypeError #54769

Closed
glaubinix opened this issue Apr 29, 2024 · 7 comments
Closed

[Messenger] schema assets filter causes TypeError #54769

glaubinix opened this issue Apr 29, 2024 · 7 comments

Comments

@glaubinix
Copy link
Contributor

glaubinix commented Apr 29, 2024

Symfony version(s) affected

6.4.7

Description

This commit introduced a new schema asset filter that filters anything that isn't the doctrine messenger table. However, the filter function can receive other types than string based on this doctrine code the filter can be string|AbstractAsset. This can cause a type error in PostgreSQL and potentially other databases.

TypeError: Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection::Symfony\Component\Messenger\Bridge\Doctrine\Transport\{closure}(): Argument #1 ($tableName) must be of type string, Doctrine\DBAL\Schema\Sequence given
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/Connection.php:301
/home/runner/work/project/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:377
/home/runner/work/project/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:193
/home/runner/work/project/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:1648
/home/runner/work/project/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:1663
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/Connection.php:538
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/Connection.php:302
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/PostgreSqlConnection.php:97
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/Connection.php:434
/home/runner/work/project/vendor/symfony/doctrine-bridge/SchemaListener/AbstractSchemaListener.php:39
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/Connection.php:352
/home/runner/work/project/vendor/symfony/doctrine-messenger/Transport/DoctrineTransport.php:89
/home/runner/work/project/vendor/symfony/doctrine-bridge/SchemaListener/MessengerTransportDoctrineSchemaListener.php:43
/home/runner/work/project/vendor/symfony/doctrine-bridge/ContainerAwareEventManager.php:63
/home/runner/work/project/vendor/doctrine/orm/src/Tools/SchemaTool.php:421
/home/runner/work/project/vendor/doctrine/orm/src/Tools/SchemaTool.php:123
/home/runner/work/project/vendor/doctrine/orm/src/Tools/SchemaTool.php:101

How to reproduce

Have a Symfony doctrine messenger setup on a PostgreSQL database that uses sequences and call the MessengerTransportDoctrineSchemaListener

Possible Solution

Change the typehint to $configuration->setSchemaAssetsFilter(fn (string|AbstractAsset $tableName) => $tableName === $this->configuration['table_name']);

Additional Context

No response

@glaubinix glaubinix added the Bug label Apr 29, 2024
@glaubinix glaubinix changed the title Messenger: schema assets filter causes TypeError [Messenger] schema assets filter causes TypeError Apr 29, 2024
@hhamon
Copy link
Contributor

hhamon commented Apr 29, 2024

Looks like I'm facing a similar issue after just upgrading Symfony to 6.4.7 on one of my clients projects. Our CI fails and the following error got reported to our Sentry:

Error thrown while running command "app:redacted-command-name --no-interaction". Message: "Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection::Symfony\Component\Messenger\Bridge\Doctrine\Transport{closure}(): Argument #1 ($tableName) must be of type string, Doctrine\DBAL\Schema\Sequence given"

The new patch release introduced a BC break.

@nicolas-grekas
Copy link
Member

A regression maybe, a BC break nope ;)

@hhamon
Copy link
Contributor

hhamon commented Apr 29, 2024

A regression maybe, a BC break nope ;)

You're right! It does sound to be more like a regression than a BC break here with a the callable accepting string|AbstractAsset depending on the configured DBAL context.

@xabbuh
Copy link
Member

xabbuh commented Apr 29, 2024

see #54775

@slappyslap
Copy link

Symfony 7.0 also affected

@fabpot fabpot closed this as completed May 1, 2024
fabpot added a commit that referenced this issue May 1, 2024
…schemas (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[Messenger] accept AbstractAsset instances when filtering schemas

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #54769
| License       | MIT

Commits
-------

4d2679d accept AbstractAsset instances when filtering schemas
@99hops
Copy link

99hops commented May 5, 2024

Saw this after 6.4.7 bin/console messenger:setup-transports

console.CRITICAL: Error thrown while running command "messenger:setup-transports". 
Message: "Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection::Symfony\Component\Messenger\Bridge\Doctrine\Transport\{closure}(): Argument #1 ($tableName) must be of type string, Doctrine\DBAL\Schema\Sequence given" 

Should we wait for the next release?

@derrabus
Copy link
Member

derrabus commented May 5, 2024

Yes, or try out the dev branch.

fabpot added a commit that referenced this issue Jun 9, 2024
… (chriskapp)

This PR was submitted for the 7.2 branch but it was squashed and merged into the 6.4 branch instead.

Discussion
----------

[Messenger] Added postgres asset filter integration test

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | #54769
| License       | MIT

This PR just adds an additional postgres integration test to test the already fixed issue #54769 regarding the asset filter to prevent those issues in the future.

Commits
-------

fcad142 [Messenger] Added postgres asset filter integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants