Skip to content
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

CS Cleanup #162

Merged
merged 1 commit into from
Dec 1, 2015
Merged

CS Cleanup #162

merged 1 commit into from
Dec 1, 2015

Conversation

dereuromark
Copy link
Member

Cleanup as follow up on #161

@josegonzalez
Copy link
Member

Worth making a release (1.5.1) ?

@dereuromark
Copy link
Member Author

Probably because of hasIndexByName() which is using a wrong variable.
But it is not as urgend as 1.5 was.

@@ -60,7 +60,7 @@ public function templateData()

$action = $this->detectAction($className);

if ($action === null) {
if (empty($action)) {
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the signature of that method. It now returns an empty array instead of two different return types which is usually a bad thing and a code smell. Instead it should always return array.
In light of PHP7 and "never return more than a single one - or null as 2nd default is not acceptable anymore" this is a necessary change IMO.

@HavokInspiration HavokInspiration added this to the 1.5.1 milestone Dec 1, 2015
HavokInspiration added a commit that referenced this pull request Dec 1, 2015
@HavokInspiration HavokInspiration merged commit 800d4f7 into master Dec 1, 2015
@HavokInspiration HavokInspiration deleted the master-cleanup branch December 1, 2015 20:29
@HavokInspiration
Copy link
Member

I'll do the release for the fix to hasIndexByName()

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