Enhancement: Return types and general clean up #523

BackEndTea · 2017-11-07T12:36:36Z

This PR adds return types to a lot of functions. Functions that could also return null/ other types of responses haven't been updated.

It also forces some types on parameters where possible. And updates some docblocks

localheinz · 2017-11-07T14:49:40Z

.php_cs

@@ -15,6 +15,7 @@ $config = PhpCsFixer\Config::create()
        'no_unused_imports' => true,
        'ordered_imports' => true,
        'psr0' => false,
+        'single_blank_line_before_namespace' => true,


@BackEndTea

What do you think about enabling this fixer in a separate PR (similar to - for example - #524)?

Personally, I like to do this in two steps:

enable the fixer and commit (see the build fail)

run make cs and commit (make the build pass)

A side effect of this is that I wait with enabling a fixer until I see a PR opened that indicates that a project could benefit from enabling a specific rule (for example, I spotted here that - while consistent in context of this PR - spacing is applied before : in return type declarations, but both the proposed PSR-12 as well as the default configuration for the return_type_declaration fixer indicate that no space should be used, so it's a good case for enabling the fixer on an existing project). 🤓

localheinz · 2017-11-07T14:56:22Z

classes/Application/Speakers.php

@@ -63,7 +63,7 @@ public function findProfile()
     * @return Talk
     * @throws NotAuthorizedException
     */
-    public function getTalk($talkId)
+    public function getTalk(int $talkId)


I'm not entirely sure whether we have filtering in place that sorts out passing in a possibly integerish string value via the UI, which if we didn't would fail with a TypeError. What do you think, do we need to add missing tests here?

Also, what do think about adding/updating the corresponding @params in the docblocks?

I checked where this function is called, and its only used in one place (outside of automated tests), which is the talk view action. And there it gets filtered to an int anyways.

So if we would call this from another place in the future it should enforce some validation as well.

localheinz · 2017-11-07T14:59:02Z

classes/Domain/Model/Airport.php

@@ -15,7 +15,7 @@ class Airport extends Eloquent implements AirportInformationDatabase
     * @return Airport


Should we update docblocks here and in other places as well?

I would like to refer to zendframework/zend-diactoros#37 (comment).

What do you think, @BackEndTea?

Yeah i'll get around to updating that.

I guess this is also a time to wonder how much the docblocks are needed, if we can paint the whole picture with type declarations.

localheinz · 2017-11-07T14:59:34Z

classes/Domain/Model/TalkComment.php

@@ -1,6 +1,5 @@
 <?php

-


Looks like we definitely have a good case for enabling the single_blank_line_before_namespace fixer!

👍

localheinz · 2017-11-07T15:01:41Z

classes/Util/Immutable.php

-
-use Exception;
-
-trait Immutable


What do you think about cherry-picking cf62549 and consequently removing

Immutable

ImmutableObject

both of which are in fact unused - as well as

ImmutableTest

in a separate PR?

While I think all of the changes proposed in this PR here are good, I tend to prefer smaller PRs addressing single issues, as they are easier to review (and thus make it faster into master). Hope you don't mind me expressing this concern.

Have to admit i had to figure out how to cherry pick a commit, but this is now done in #528

localheinz · 2017-11-07T15:53:19Z

@BackEndTea

This needs a good rebase after all of your other outstanding PRs have been merged!

$ git checkout master
$ git fetch upstream master
$ git merge upstream/master
$ git push origin master

then

$ git checkout enhancement/php7-features
$ git rebase master
$ git push -f origin enhancement/php7-features

BackEndTea · 2017-11-07T15:57:59Z

@localheinz I'll get to that once @chartjes has finished most PR's.

Added return types and type declarations where possible in Speakers

Removed double whitespace before the namespace which doesn't get picked up by phpcs, and added return types

- Added whitespace between <?php and the namespace for consistency - Added return types to Form functions - Removed useless comments

This rule has already been applied just about everywhere in the code base, so it might be better to enforce it for consistency.

This function is used in exactly one place, the view action of the talk controller, where the talk id is filtered to an int. So we should be okay to force this as an int.

localheinz mentioned this pull request Nov 7, 2017

Enhancement: Enable return_type_declaration fixer #524

Merged

2 tasks

localheinz reviewed Nov 7, 2017

View reviewed changes

BackEndTea added 10 commits November 8, 2017 08:56

Update Speakers to use php 7 features

dd8b02b

Added return types and type declarations where possible in Speakers

Update Models

ca60519

Update Domain classes

ba5d656

Removed double whitespace before the namespace which doesn't get picked up by phpcs, and added return types

Update Form classes

f921174

- Added whitespace between <?php and the namespace for consistency - Added return types to Form functions - Removed useless comments

Add return types to test helpers

1e62d29

Add single blank line before namespace cs rule

531dff1

This rule has already been applied just about everywhere in the code base, so it might be better to enforce it for consistency.

Apply cs fixes

b05e333

Update doc blocks in the airport model

d123cd0

Update return types in test case

d1a1f73

Update docblock of getTalk

1d19a45

This function is used in exactly one place, the view action of the talk controller, where the talk id is filtered to an int. So we should be okay to force this as an int.

chartjes merged commit 1d19a45 into opencfp:master Nov 8, 2017

BackEndTea deleted the enhancement/php7-features branch December 14, 2017 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: Return types and general clean up #523

Enhancement: Return types and general clean up #523

BackEndTea commented Nov 7, 2017 •

edited

localheinz Nov 7, 2017

localheinz Nov 7, 2017

BackEndTea Nov 8, 2017

localheinz Nov 7, 2017

BackEndTea Nov 7, 2017

localheinz Nov 7, 2017

localheinz Nov 7, 2017

BackEndTea Nov 7, 2017

localheinz commented Nov 7, 2017 •

edited

BackEndTea commented Nov 7, 2017

		@@ -15,7 +15,7 @@ class Airport extends Eloquent implements AirportInformationDatabase
		* @return Airport

Enhancement: Return types and general clean up #523

Enhancement: Return types and general clean up #523

Conversation

BackEndTea commented Nov 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

localheinz commented Nov 7, 2017 • edited

BackEndTea commented Nov 7, 2017

BackEndTea commented Nov 7, 2017 •

edited

localheinz commented Nov 7, 2017 •

edited