Skip to content

[Config] Builder: Remove typehints and allow for EnvConfigurator #40903

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 26, 2021
Merged

[Config] Builder: Remove typehints and allow for EnvConfigurator #40903

merged 1 commit into from
Apr 26, 2021

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Apr 22, 2021 •
edited
Loading

Q A
Branch? 5.x
Bug fix? yes -- maybe
New feature? no -- maybe =)
Deprecations? no
Tickets
License MIT
Doc PR

When writing documentation we found that we don't really support environment variables in the leaves. Ie, we expect a boolean but you provide "%env(ENABLE_FOO)%"

This PR will also introduce ParamConfigurator to allow parameters to be passed as config.

The changes to the generated code:

    /**
+    * @param bool|ParamConfigurator $value
     * @default false
     * @return $this
     */
-   public function enabled(bool $value): self
+   public function enabled($value): self
    {
        $this->enabled = $value;
    
        return $this;
    }

@nicolas-grekas
Copy link
Member

This makes me realize that any %param% could also be allowed there, not only env ones.
This means we need to add a ParamConfigurator, and that EnvConfigurator should extend it.
Then, the param() function should return a ParamConfigurator.

@nicolas-grekas nicolas-grekas added this to the 5.3 milestone Apr 22, 2021
@Nyholm
Copy link
Member Author

Nyholm commented Apr 22, 2021 •
edited
Loading

I cannot figure out why I get this error:

1) Symfony\Component\Config\Tests\Builder\GeneratedConfigTest::testConfig with data set "Placeholders" ('Placeholders', 'placeholders')
Error: Call to undefined function Symfony\Component\DependencyInjection\Loader\Configurator\env()

src/Symfony/Component/Config/Tests/Builder/Fixtures/Placeholders.config.php:7
src/Symfony/Component/Config/Tests/Builder/GeneratedConfigTest.php:50

@Nyholm Nyholm mentioned this pull request Apr 22, 2021
Merged
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 22, 2021 •
edited
Loading

That's because the function is not autoloaded. This means that we should force-load ContainerConfigurator in the loader.

@Nyholm
Copy link
Member Author

Nyholm commented Apr 22, 2021 •
edited
Loading

I tried that before, but I failed "force loading it". Hm.. it works now. I think I have to blame this on incompetence.

But this means that a user will have the same issues. Before the ConfigBuilders, this was not an issue because the user would always use ContainerConfigurator.

@Nyholm Nyholm force-pushed the env-support branch 2 times, most recently from 2a1e163 to 77206e2 Compare April 22, 2021 13:14
@Nyholm Nyholm requested a review from stof April 22, 2021 13:14
ro0NL
ro0NL approved these changes Apr 23, 2021
View reviewed changes
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

🚀

@Nyholm Nyholm force-pushed the env-support branch 2 times, most recently from 84d1506 to 45c99c2 Compare April 23, 2021 11:56
@Nyholm
Copy link
Member Author

Nyholm commented Apr 24, 2021

The PR is rebased. Travis and Fabbot are both reporting on unrelated issues.

@derrabus
Copy link
Member

Thank you Tobias.

@derrabus derrabus merged commit 88abb39 into symfony:5.x Apr 26, 2021
@Nyholm
Copy link
Member Author

Nyholm commented Apr 26, 2021

Thank you for all the reviews and the merge.

@Nyholm Nyholm deleted the env-support branch April 26, 2021 21:40
@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

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof approved these changes

@ro0NL ro0NL ro0NL approved these changes

@derrabus derrabus derrabus approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@Nyholm @nicolas-grekas @derrabus @stof @ro0NL @carsonbot