Skip to content

[Fix] Avoid break on assert if security node is missing in config #249

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

Merged

Conversation

cvergne
Copy link
Contributor

@cvergne cvergne commented Jun 6, 2025

Hi,

I was looking to fix #247 (but this PR is not related to the issue), so I've tried to install the bundle on a fresh new (light) Symfony.

Then when the cache:clear occurs, I had the following error :

In GraphQLiteExtension.php line 78:

  [ErrorException]
  Warning: Undefined array key "security"

The problem is the assert about the security node.
But that node is not mandatory as we have default values for enable_login & enable_me (and as the doc says, we have to set them only if we want to customize).
But then I noticed the assert was required by PHPStan level, so the fix should be to set an empty array for security if none is already set.

Note

I will open an other following PR about the CI because currently assert are bypassed by the PHP used in CI, which is why tests succeed.

@cvergne cvergne force-pushed the fix/assert-security-config branch from 3493886 to e22df6c Compare June 6, 2025 09:38
@cvergne cvergne changed the title [Fix] Remove assert for security node in config as node is not mandatory [Fix] Break on assert if security node is missing in config Jun 6, 2025
@cvergne cvergne changed the title [Fix] Break on assert if security node is missing in config [Fix] Avoid break on assert if security node is missing in config Jun 6, 2025
@andrew-demb
Copy link
Collaborator

I think the best way to solve such problems is to use addDefaultsIfNotSet [1] in the Configuration class, but let's not apply this suggestion in the current PR.

I'm OK with the current approach for now.

[1] https://symfony.com/doc/current/components/config/definition.html#:~:text=is%20also%20called).-,addDefaultsIfNotSet,-()

@andrew-demb andrew-demb merged commit e65189c into thecodingmachine:master Jun 6, 2025
3 checks passed
@andrew-demb
Copy link
Collaborator

Thank you @cvergne

@cvergne
Copy link
Contributor Author

cvergne commented Jun 6, 2025

I think the best way to solve such problems is to use addDefaultsIfNotSet [1] in the Configuration class, but let's not apply this suggestion in the current PR.

🤦 You're totally right. I'll set up a new PR for a future release with the fix in the Configuration ;)

@cvergne cvergne deleted the fix/assert-security-config branch June 9, 2025 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

method "setNotMappedTypes()" does not exist
2 participants