Skip to content

Conversation

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Mar 11, 2020

Summary of Changes

I was very tired this evening so I did what any sane person would normally do when they're tired. Get inspired by their editor to go cleanup doc blocks, return types etc in all the Joomla controllers in J4 (this wasn't automated before those moan based on the title - i just used the highlighting as a point to review)

Testing Instructions

Code Review

Documentation Changes Required

None. This improves the incode documentation to match reality

@wilsonge wilsonge changed the title Cleanup of controller based on Intellij highlighting [4.0] Cleanup of controller based on Intellij highlighting Mar 11, 2020
* Override parent add method.
*
* @return mixed True if the record can be added, a \Exception object if not.
* @return \Exception|void True if the record can be added, a \Exception object if not.
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup :D I didn’t dare follow the rabbit hole this late but one to come back to

* @param array $urlparams An array of safe URL parameters and their variable types, for valid values see {@link \JFilterInput::clean()}.
*
* @return static This object to support chaining.
* @return static|boolean This object to support chaining.
Copy link
Contributor

Choose a reason for hiding this comment

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

Stuff like this makes me realize a lot of the Joomla code would choke up a static analyzer.

  1. I'm pretty sure the base display() method in the controllers only documents a $this return, no type covariance

  2. I'm pretty sure all of the controller returns are unused anyway, either stuff is being echoed thanks to the output buffer being started or it's just all ignored

  3. Seriously, this return type just makes me sad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’d like to move a lot of it to exceptions or in many of these cases as we are just setting redirects into the controller probably return $this would work. But for now I’m just documenting correctly can do a second round of cleanup specifically on that

@wilsonge wilsonge merged commit a79bb14 into joomla:4.0-dev Mar 11, 2020
@wilsonge wilsonge deleted the feature/controller-cleanup branch March 11, 2020 00:30
@wilsonge wilsonge added this to the Joomla 4.0 milestone Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants