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

Use tab attribute as fallback when a module does not have category #16658

Merged
merged 5 commits into from Dec 13, 2019

Conversation

@PierreRambaud
Copy link
Contributor

PierreRambaud commented Dec 2, 2019

Questions Answers
Branch? develop
Description? Stop using the Addons Api to request module categories. We already have a yaml file with all information.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15298
How to test? Follow ticket instruction + nothing is broken in the MBO module.

This change is Reviewable

@PierreRambaud PierreRambaud requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 2, 2019
Copy link
Contributor

matthieu-rolland left a comment

LGTM, but it seems that travis is not happy, not sure if it's about this PR or travis being annoying...

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15298 branch from 40533a8 to 6abd947 Dec 9, 2019
Copy link
Contributor

jolelievre left a comment

Now that there are less dependencies in this class Maybe it could be good, and not too long, to add a few unit tests?

*/
private function createMenuObject($menu, $name, $moduleIds = [], $tab = null)
private function createMenuObject(string $menu, string $name, array $moduleIds = [], ?string $tab = null): stdClass
{
return (object) array(

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 9, 2019

Contributor

🤮 isn't it possible to create a class to manage this object?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 10, 2019

Author Contributor

Little bit overkill because it create and need an stdClass :/

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 10, 2019

Author Contributor

But I'll add unit tests

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 10, 2019

Contributor

You're right it's probably overkill, it just feel weird to use and object, if we don't use strong typed object I guess just using an array would do the job no? This would avoid weird syntaxes like $categories['categories']->subMenu[self::CATEGORY_THEME]->modules and the class could use $categories = json_decode(json_encode($categories)); right before the return (since it's already present in the class)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 11, 2019

Author Contributor

This was the original behaviour, and I can make a json_deciode / json_encode because we store Module objects into the modules key 😅
Playing with collection would have been much better :(

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 11, 2019

Contributor

Yeah that's why I suggested creating a real class, but I guess this will be an improvement for later

@PierreRambaud PierreRambaud added the WIP label Dec 10, 2019
@PierreRambaud PierreRambaud removed the WIP label Dec 10, 2019
@PierreRambaud

This comment has been minimized.

Copy link
Contributor Author

PierreRambaud commented Dec 10, 2019

@jolelievre unit tests added

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15298 branch from fe5c03c to d2827e3 Dec 11, 2019
Copy link
Contributor

jolelievre left a comment

Thanks @PierreRambaud

@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Dec 11, 2019
@PierreRambaud PierreRambaud merged commit a8a1e0f into PrestaShop:develop Dec 13, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud PierreRambaud deleted the PierreRambaud:fix/15298 branch Dec 13, 2019
@matks matks added this to the 1.7.7.0 milestone Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.