8000 [Config] finish adding parameter types by Tobion · Pull Request #32286 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] finish adding parameter types #32286

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
Jul 5, 2019
Merged

[Config] finish adding parameter types #32286

merged 1 commit into from
Jul 5, 2019

Conversation

Tobion
Copy link
Contributor
@Tobion Tobion commented Jun 29, 2019
Q A
Branch? master
Bug fix? yno
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32179
License MIT
Doc PR symfony/symfony-docs#...

Completes some missing things in #32201

@Tobion Tobion mentioned this pull request Jun 29, 2019
Merged
@Tobion Tobion force-pushed the config-param-types branch from 1c8f52a to cd07c1a Compare June 29, 2019 19:25
derrabus
derrabus requested changes Jun 29, 2019
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Your changes make certain other components in version 5 incompatible with Config 4.4. Im not sure if we should break that compatibility just for adding type-hints.

@Tobion Tobion force-pushed the config-param-types branch from cd07c1a to 1905db9 Compare June 29, 2019 19:55
@derrabus derrabus mentioned this pull request Jun 29, 2019
Closed
@Tobion Tobion mentioned this pull request Jun 29, 2019
Merged
@Tobion Tobion force-pushed the config-param-types branch from 1905db9 to 2ca8354 Compare June 29, 2019 22:48
@Tobion
Copy link
Contributor Author
Tobion commented Jun 29, 2019

Fixed dependencies. This will happen anyway eventually with all the types added in master.

@Tobion Tobion added this to the 5.0 milestone Jun 29, 2019
Tobion added a commit that referenced this pull request Jul 2, 2019
@Tobion
minor #32288 fix invalid call to PhpFileLoader::load() in a test (Tob…
205cd7a 
…ion)

This PR was merged into the 4.3 branch.

Discussion
----------

fix invalid call to PhpFileLoader::load() in a test

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yno
| New feature?  | yno <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |
| License       | MIT
| Doc PR        |

This was calling `\Symfony\Component\DependencyInjection\Loader\PhpFileLoader::load($resource, string $type = null)` with the container object as $type which I found in #32286

Commits
-------

8930335 fix invalid call to PhpFileLoader::load() in a test
nicolas-grekas
nicolas-grekas reviewed Jul 3, 2019
View reviewed changes
src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php
@@ -417,6 +411,10 @@ protected function createNode()
}

if ($this->default) {
if (!\is_array($this->defaultValue)) {
Copy link
Member

Choose a reason for hiding this comment

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

only for 5? missing deprecation in 4.4?

Copy link
Contributor Author
@Tobion Tobion Jul 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just moved the check from PrototypedArrayNode.php::setDefaultValue here so the typhint in setDefaultValue can be added. The $this->defaultValue comes from a generic setter that we cannot use types for. The behavior is the same, just cleaner code using types when possible.

nicolas-grekas
nicolas-grekas reviewed Jul 3, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm good with the bump of lowest versions. Please move all updates to docblocks to a PR on a lower branch.

@Tobion
Copy link
Contributor Author
Tobion commented Jul 3, 2019

Please move all updates to docblocks to a PR on a lower branch.

I can do it for the single @return ExtensionInterface I added for autocomplete. But most other changes are about removing phpdocs that are obsolete with type hints. Those we should not apply in lower branches, because the type hints are not there yet.

@fabpot fabpot mentioned this pull request Jul 5, 2019
Closed
fabpot added a commit that referenced this pull request Jul 5, 2019
@fabpot
minor #32382 Fix phpdocs (fabpot)
d292b13 
This PR was merged into the 4.4 branch.

Discussion
----------

Fix phpdocs

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Backported from #32286

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

4acddef fixed phpdocs
fabpot
fabpot approved these changes Jul 5, 2019
View reviewed changes
@fabpot
Copy link
Member
fabpot commented Jul 5, 2019

Thank you @Tobion.

@fabpot fabpot merged commit 2ca8354 into symfony:master Jul 5, 2019
fabpot added a commit that referenced this pull request Jul 5, 2019
@fabpot
minor #32286 [Config] finish adding parameter types (Tobion)
bfabad4 
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Config] finish adding parameter types

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yno
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Completes some missing things in #32201

Commits
-------

2ca8354 [Config] finish adding parameter types
@Tobion Tobion deleted the config-param-types branch July 5, 2019 17:08
@derrabus derrabus mentioned this pull request Jul 5, 2019
Closed
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@derrabus derrabus derrabus requested changes

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

Successfully merging this pull request may close these issues.
5 participants
@Tobion @fabpot @nicolas-grekas @derrabus @carsonbot
0