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

Refactor Dispatcher::useDefaultController() #9066

Merged
merged 13 commits into from Jun 18, 2018

Conversation

Projects
None yet
6 participants
@michaelKaefer
Contributor

michaelKaefer commented May 13, 2018

Questions Answers
Branch? develop
Description? Cleaner and more testable code
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? -
How to test? -

This change is Reviewable

Refactor Dispatcher::useDefaultController()
Cleaner and more testable code

@prestonBot prestonBot added the develop label May 13, 2018

Refactor Dispatcher::useDefaultController()
Cleaner and more testable code
*
* @return string
*/
private function useDefaultController()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 15, 2018

Contributor

you have to keep this function "public" to not break semver :)

This comment has been minimized.

@michaelKaefer
*
* @return null|string
*/
public function getDefaultTabClassName()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 15, 2018

Contributor

I don't see why you add this, in the context of Dispatcher refactoring : what I'm missing here? 🤔

This comment has been minimized.

@michaelKaefer

michaelKaefer May 19, 2018

Contributor

@mickaelandrieu It's meant to get the default tab name from the model ($employee->getDefaultTabClassName()) inside the Dispatcher::setDefaultController() method

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 25, 2018

Hi, can you rebase please? :)

*
* @return string
*/
private function setDefaultController($frontControllerType, Employee $employee = null)

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 15, 2018

Member

I'd rather write it as a getter (getDefaultController). Even if set a value somewhere, we basically ask the method for returning the name of the Default controller.

setDefaultController is a non-sense to me as we do not send any value about a default controller to store in $this->default_controller.

return $this->default_controller = 'default';
break;
default:
return $this->default_controller = 'index';

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 15, 2018

Member

Another review would be interesting, but what about splitting the value affectation and the `return [...]; at the end of the method?

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 17, 2018

@Quetzacoalt91 I made commits to change what you have reviewd. Thank you 👍

}
}
// Default
$defaultController = 'AdminDashboard';

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 18, 2018

Member

Be careful, now you removed the return statement, this default value will override $defaultController = $tabClassName;

To avoid this, you can move this line after the case:

case self::FC_ADMIN:
    // Default
    $defaultController = 'AdminDashboard';
    [...]
@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 18, 2018

@Quetzacoalt91 Thank you! I fixed this stupid mistake of mine!

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

Thanks @michaelKaefer

@mickaelandrieu mickaelandrieu merged commit e53d97f into PrestaShop:develop Jun 18, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@michaelKaefer michaelKaefer deleted the michaelKaefer:patch-5 branch Jun 18, 2018

@eternoendless eternoendless added this to the 1.7.5.0 milestone Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment