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

Fixed mismatch of PHPDocs + unit tests #10038

Merged
merged 10 commits into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Aug 23, 2018

Questions Answers
Branch? develop
Description? While reviewing the pull request on HookDispatcherInterface, @jolelievre noticed some mismatch in the implementation of the HookDispatcher: sometimes a content was expected a string, sometimes an array... now the expectations are strict and the behavior covered 👍
Type? bug fix
Category? TE
BC breaks? yes, it is but it's not impacting PrestaShop developers/users.
Deprecations? no
How to test? Nothing to test, tests should pass.

This change is Reviewable

@@ -175,7 +182,7 @@ public function dispatchWithParameters($hookName, array $hookParameters = [])
*/
public function dispatchRendering(HookInterface $hook)
{
$event = $this->hookDispatcherService->renderForParameters(
$event = $this->renderForParameters(

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 23, 2018

Contributor

this function is never used in the Core, but still let's fix it 👼

@mickaelandrieu

mickaelandrieu Aug 23, 2018

Contributor

this function is never used in the Core, but still let's fix it 👼

@jolelievre

not sure exactly how Reviews work in github :P

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

Hey fellows, it was a complete nightmare to fix this wrong behavior as it was used almost everywhere :/

When a bug is "used as a feature", it's hard to fix but I guess we are good here...

ping @Quetzacoalt91: did I missed something?

Contributor

mickaelandrieu commented Aug 24, 2018

Hey fellows, it was a complete nightmare to fix this wrong behavior as it was used almost everywhere :/

When a bug is "used as a feature", it's hard to fix but I guess we are good here...

ping @Quetzacoalt91: did I missed something?

;
return empty($hookRenders) ? '' : implode('<br class="hook-separator" />', $hookRenders);

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

time to remove this ugly hack: add an HTML separator between 2 modules displaying an hook?

I mean: seriously? o_O

@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

time to remove this ugly hack: add an HTML separator between 2 modules displaying an hook?

I mean: seriously? o_O

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

not sure exactly how Reviews work in github :P

I can show you on monday

Also, the next time you see a mismatch between PHPDocs and code... you FIX IT :trollface:

Contributor

mickaelandrieu commented Aug 24, 2018

not sure exactly how Reviews work in github :P

I can show you on monday

Also, the next time you see a mismatch between PHPDocs and code... you FIX IT :trollface:

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 27, 2018

Contributor

@PierreRambaud comments addressed!

Contributor

mickaelandrieu commented Aug 27, 2018

@PierreRambaud comments addressed!

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 27, 2018

Contributor

LGTM @PierreRambaud? I don't see how QA can validate this

Contributor

mickaelandrieu commented Aug 27, 2018

LGTM @PierreRambaud? I don't see how QA can validate this

@PierreRambaud PierreRambaud merged commit de96008 into PrestaShop:develop Aug 27, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the mickaelandrieu:fix/undefined-service branch Aug 27, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 27, 2018

Contributor

QA can't validate. So I merged. Thanks a lot @mickaelandrieu

Contributor

PierreRambaud commented Aug 27, 2018

QA can't validate. So I merged. Thanks a lot @mickaelandrieu

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