Skip to content

[Form] Static call TimezoneType::getTimezones #23652

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
merged 1 commit into from
Jul 25, 2017
Merged

[Form] Static call TimezoneType::getTimezones #23652

merged 1 commit into from
Jul 25, 2017

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Jul 24, 2017

Q A
Branch? 3.2
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

Spotted in #23648

@ogizanagi
Copy link
Contributor

Seems like you've made fabbot unhappy though. It has feelings you know.
(You should really use the full PR template, even for minor patches.)

@@ -66,7 +66,7 @@ public function loadChoiceList($value = null)
return $this->choiceList;
}

return $this->choiceList = new ArrayChoiceList($this->getTimezones(), $value);
return $this->choiceList = new ArrayChoiceList(self::getTimezones(), $value);
Copy link
Member

Choose a reason for hiding this comment

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

Technically, the previous code allowed extending the class to provide alternative implem via inheritance. Was it on purpose? Can't say right now, but that should be considered before approval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of 3.0 getTimezones is a private static.. so not open for inheritance :)

Copy link
Member

Choose a reason for hiding this comment

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

ok, strange, but verified: https://3v4l.org/n61o4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah like so.. you're right i didnt checked it.. i assumed it :)

Copy link
Member

Choose a reason for hiding this comment

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

OH, but you can turn it protected, so still open for extension!
https://3v4l.org/f2STp

Copy link
Contributor Author

@ro0NL ro0NL Jul 25, 2017

Choose a reason for hiding this comment

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

Right. I cant imagine this is done on purpose.. at least core doesnt seem to rely on it elsewhere.

Nice catch btw :)

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Jul 25, 2017
@ogizanagi
Copy link
Contributor

Thanks @ro0NL.

@ogizanagi ogizanagi merged commit fe48ab1 into symfony:3.2 Jul 25, 2017
ogizanagi added a commit that referenced this pull request Jul 25, 2017
This PR was merged into the 3.2 branch.

Discussion
----------

[Form] Static call TimezoneType::getTimezones

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

Spotted in #23648

Commits
-------

fe48ab1 [Form] Static call TimezoneType::getTimezones
@ro0NL ro0NL deleted the patch-1 branch July 26, 2017 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants