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

Added a new collector for Hooks #8327

Merged
merged 2 commits into from Sep 18, 2017

Conversation

Projects
None yet
5 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Sep 13, 2017

Questions Answers
Branch? develop
Description? We now have information about all dispatched hooks in web debug toolbar on Symfony pages.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? partially, I need it to make a good migration and ensure I dont remove (too much?) hooks!
How to test? Access a Symfony page

I've finally removed the dumped values: for a lot of parameters and objects the time to render the profiler become too long, I think it's up to the developer to dump data in their modules, now they have the right information on modules and hooks dispatched.

Some captures

hook

hook

hooks_and_modules

@PrestaShop PrestaShop deleted a comment from prestonBot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@mickaelandrieu mickaelandrieu changed the title from [WIP] Added a new collector for Hooks to Added a new collector for Hooks Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@mickaelandrieu mickaelandrieu referenced this pull request Sep 14, 2017

Merged

Migrate Performance page to Symfony #8279

13 of 13 tasks complete
Show outdated Hide outdated classes/Hook.php
Show outdated Hide outdated src/PrestaShopBundle/DataCollector/HookDataCollector.php
*/
public function getName()
{
return 'ps.hooks_collector';

This comment has been minimized.

@eternoendless

eternoendless Sep 14, 2017

Member

I think this value should be stored in a constant

@eternoendless

eternoendless Sep 14, 2017

Member

I think this value should be stored in a constant

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 14, 2017

Contributor

why? this value is only reused on service declaration.

@mickaelandrieu

mickaelandrieu Sep 14, 2017

Contributor

why? this value is only reused on service declaration.

This comment has been minimized.

@eternoendless

eternoendless Sep 15, 2017

Member

The way I see it, constants are not (only) about reuse, they are also about not having important pieces of information stored as literals buried in the code.

One of the advantages of constants is that they are declared at the top of the class in an organized manner, which in my opinion improves clarity.

@eternoendless

eternoendless Sep 15, 2017

Member

The way I see it, constants are not (only) about reuse, they are also about not having important pieces of information stored as literals buried in the code.

One of the advantages of constants is that they are declared at the top of the class in an organized manner, which in my opinion improves clarity.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 18, 2017

Contributor

Ok, this don't apply here, this information is totaly useless for the developer and will increase what we call "visual debt" :)

@mickaelandrieu

mickaelandrieu Sep 18, 2017

Contributor

Ok, this don't apply here, this information is totaly useless for the developer and will increase what we call "visual debt" :)

Show outdated Hide outdated src/PrestaShopBundle/DataCollector/HookDataCollector.php
Show outdated Hide outdated src/PrestaShopBundle/DataCollector/HookRegistry.php
Show outdated Hide outdated src/PrestaShopBundle/DataCollector/HookRegistry.php
Show outdated Hide outdated src/PrestaShopBundle/DataCollector/HookRegistry.php
Show outdated Hide outdated src/PrestaShopBundle/DataCollector/HookRegistry.php
</tbody>
</table>
{% endfor %}
{% endmacro %}

This comment has been minimized.

@eternoendless

eternoendless Sep 14, 2017

Member

Missing LF here 🙂

@eternoendless

eternoendless Sep 14, 2017

Member

Missing LF here 🙂

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 18, 2017

Contributor

fixed

@mickaelandrieu

mickaelandrieu Sep 18, 2017

Contributor

fixed

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 18, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Sep 18, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 7
- Added 8
           

Complexity increasing per file
==============================
- classes/Hook.php  9
         

See the complete overview on Codacy

codacy-bot commented Sep 18, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 7
- Added 8
           

Complexity increasing per file
==============================
- classes/Hook.php  9
         

See the complete overview on Codacy

abstract class ModuleCore
abstract class ModuleCore implements ModuleInterface

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 18, 2017

Member

Why do we add this interface? If the legacy class is supposed to disappear, I do not understand this change.

@Quetzacoalt91

Quetzacoalt91 Sep 18, 2017

Member

Why do we add this interface? If the legacy class is supposed to disappear, I do not understand this change.

@Quetzacoalt91 Quetzacoalt91 merged commit 9f785c6 into PrestaShop:develop Sep 18, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91
Member

Quetzacoalt91 commented Sep 18, 2017

Thank you @mickaelandrieu

@xBorderie xBorderie added this to the 1.7.3.0 milestone Sep 18, 2017

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