8000 [Config] Support extensions without configuration in ConfigBuilder warmup by wouterj · Pull Request #40859 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] Support extensions without configuration in ConfigBuilder warmup #40859

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 19, 2021
Merged

[Config] Support extensions without configuration in ConfigBuilder warmup #40859

merged 1 commit into from
Apr 19, 2021

Conversation

wouterj
Copy link
Member
@wouterj wouterj commented Apr 18, 2021
Q A
Branch? 5.x
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

ConfigurationExtensionInterface::getConfiguration() is nullable.

As a real use-case: A small internal bundle in my company just uses array_merge to manage a very limited set of configuration. We don't have these fancy Configuration classes.

@wouterj wouterj requested a review from Nyholm April 18, 2021 17:48
@wouterj wouterj changed the title [Config] Support extensions without configuration [Config] Support extensions without configuration in ConfigBuilder warmup Apr 18, 2021
Nyholm
Nyholm approved these changes Apr 18, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
Did you get an exception thrown at you when warming the cache?

The goal is that no exceptions should be thrown in this stage. If you are trying to use a ConfigBuilder from your bundle (the one with no Configuration) only then you should get an exception.

Well done

@wouterj
Copy link
Member Author
wouterj commented Apr 18, 2021 •
edited
Loading

@Nyholm yes, I did (an Error though):

§ sf console cache:clear

 // Clearing the cache for the dev environment with debug true

19:51:33 CRITICAL  [php] Uncaught Error: Argument 1 passed to Symfony\Component\Config\Builder\ConfigBuilderGenerator::build() must implement interface Symfony\Component\Config\Definition\ConfigurationInterface, null given, called in /.../vendor/symfony/framework-bundle/CacheWarmer/ConfigBuilderCacheWarmer.php on line 80 ["exception" => TypeError { …}]

In ConfigBuilderGenerator.php line 43:

  Argument 1 passed to Symfony\Component\Config\Builder\ConfigBuilderGenerator::build() must implement interface Symfony\Component\Config\Definition\Configu
  rationInterface, null given, called in /.../vendor/symfony/framework-bundle/CacheWarmer/ConfigBuil
  derCacheWarmer.php on line 80


cache:clear [--no-warmup] [--no-optional-warmers]

exit status 255

This was referenced Apr 18, 2021
Merged
Closed
@Nyholm
Copy link
Member
Nyholm commented Apr 19, 2021

Thank you <3

@Nyholm Nyholm merged commit 0fd8413 into symfony:5.x Apr 19, 2021
@wouterj wouterj deleted the configbuilder-warmup-without-config branch April 19, 2021 10:41
@alexander-schranz
Copy link
Contributor
alexander-schranz commented Apr 30, 2021 •
edited
Loading

Did also get into this error when testing the beta with @sulu, @wouterj thx for fixing this 👍 . The DoctrineFixtureBundle is also one of the bundles not having a Configuration class. Should we really log a warning here when it is the case that a bundle doesn't have any configuration, I would just silently ignore it.

@Nyholm
Copy link
Member
Nyholm commented Apr 30, 2021

It is fine to silently ignore it. You will get a proper extension if you are trying to use the config builder.

@alexander-schranz
Copy link
Contributor
alexander-schranz commented Apr 30, 2021 •
edited
Loading

Did see its just an info log not a warning or error log my eyes did play a prank on me 🙈. If we want still remove the log a PR is open: #41009 feel free to close.

@fabpot fabpot mentioned this pull request May 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

Assignees
No one assigned
Labels
Bug Config Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wouterj @Nyholm @alexander-schranz @carsonbot
0