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

Moving the call to the function renderwidget #6019

Merged
merged 1 commit into from Aug 20, 2016

Conversation

frederic-benoist
Copy link
Contributor

Questions Answers
Branch? develop
Description? Simple move for futur override (necessary for profiling)
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? N/A
How to test? not for now

@Shudrum
Copy link
Contributor

Shudrum commented Aug 16, 2016

Hi!

What do you wan't to do with this?

I mean: statics are BAD, and we do not wan't to add some statics methods anymore.
If you have a REALLY good reason that cannot be resolved in a better way, why not.

@Shudrum Shudrum self-assigned this Aug 16, 2016
@frederic-benoist
Copy link
Contributor Author

Without this change, how to add profiling modules on PrestaShop 1.7?
Some examples :

  • ps_banner use renderWidget on displayHome
  • ps_contactinfo use renderWidget on displayNav1 and displayFooter
  • ps_linklist use renderWidget on displayFooter

This solution allows the creation of an override function as with the hook::coreCallHook()

@Shudrum
Copy link
Contributor

Shudrum commented Aug 16, 2016

So the main problem is that we have removed some hooks?

@frederic-benoist
Copy link
Contributor Author

No.
With the new renderWidget interface the old profiling tools can't works (hook::coreCallHook() is not used),

With this change, it's possible to override the module call (the new function hook::coreRenderWidget) and add Profiling.

@Shudrum
Copy link
Contributor

Shudrum commented Aug 19, 2016

Ok, I understand the problem and you cannot do something better and you have just followed the other static method just bellow.

It is ok for me :)

But before merging : can you remove the merge commit ? Your pull request should only contain your commit.

Thank you!

@frederic-benoist
Copy link
Contributor Author

remove merge commit done.

Thx

@Shudrum Shudrum merged commit 5f0e0b4 into PrestaShop:develop Aug 20, 2016
@Shudrum
Copy link
Contributor

Shudrum commented Aug 20, 2016

Thank you!

@Shudrum Shudrum changed the title CO : moving the call to the function renderwidget Moving the call to the function renderwidget Sep 16, 2016
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.

None yet

2 participants