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

Hook improvements #1714

Merged
merged 8 commits into from Dec 3, 2015

Conversation

Projects
None yet
6 participants
@bibich
Contributor

bibich commented Oct 5, 2015

This PR aims to simplify the use of hooks and fixes some issues.

Now, it's no longer required to implement your own class to handle hooks. If you only want to render a template or add a css or js file you can do it easily. Here is an example :

<hooks>
    <hook id="hooknavigation.hook.test">
      <tag name="hook.event_listener" event="main.navbar-primary" templates="render:myTemplate.html" />
      <tag name="hook.event_listener" event="main.stylesheet" type="front" active="1" templates="css:myCss.css;render:theme.css.html" />
    </hook>
</hooks>

You can notice that the class argument has been omitted as well as the scope argument.

The tag syntax is similar and it operates the same way, except for the new argument templates. This argument is used to define what contents are going to be injected in the hook.

The syntax :

templates="<action>:<file>[;<action>:<file>]*"
  • action (what to do with the file) :
    • render : this is the default one (if action is omitted). It will render the file. all parameters affected to the hook will be available in the template.
    • dump : will just dump the content of the file.
    • css : will create a link html tag and add the CSS file.
    • js : will create a script html tag and add the JavaScript file.
  • file : this is the full path of the file related to the root directory of your template directory. For example, if the hook is a front office hook and want to render the file myTemplate.html. This template should be located in your module inside the templates\frontOffice\default\ directory. The overriding system is the same as used with classic hook.

I've also added a command to cleanup hooks of a module or for all modules. They will be recreated (with default configuration) when the cache of the application will be cleared :

# remove module hooks of the HookNavigation module (without confirmation)
php Thelia hook:clean HookNavigation --assume-yes
# remove hooks of all modules (with confirmation)
php Thelia hook:clean

Feel free to comment this PR. It's not finished and all your remarks are welcome.

@lunika

This comment has been minimized.

Show comment
Hide comment
@lunika

lunika Oct 5, 2015

Contributor

I don't like this 👎

Contributor

lunika commented Oct 5, 2015

I don't like this 👎

@roadster31

This comment has been minimized.

Show comment
Hide comment
@roadster31

roadster31 Oct 5, 2015

Contributor

What is wrong @lunika ?

Providing shortcuts to simplify hooks management and reduce the amount of code is a nice idea 👍

Contributor

roadster31 commented Oct 5, 2015

What is wrong @lunika ?

Providing shortcuts to simplify hooks management and reduce the amount of code is a nice idea 👍

@lunika

This comment has been minimized.

Show comment
Hide comment
@lunika

lunika Oct 5, 2015

Contributor

it obfuscates how hook work, why it is interesting to declare all this amount of class and php. One of the good thing with hooks is that you can remove the use of loop and have a more traditional MVC behaviour. But this is just my point of view :-)

Contributor

lunika commented Oct 5, 2015

it obfuscates how hook work, why it is interesting to declare all this amount of class and php. One of the good thing with hooks is that you can remove the use of loop and have a more traditional MVC behaviour. But this is just my point of view :-)

@roadster31

This comment has been minimized.

Show comment
Hide comment
@roadster31

roadster31 Oct 5, 2015

Contributor

The traditional hook mechanics are still available. A module may use the shortcuts for rendering some pages and CSS, and a specific set of classes to implement sophisticated features.

Best of both worlds :)

Contributor

roadster31 commented Oct 5, 2015

The traditional hook mechanics are still available. A module may use the shortcuts for rendering some pages and CSS, and a specific set of classes to implement sophisticated features.

Best of both worlds :)

@roadster31

This comment has been minimized.

Show comment
Hide comment
@roadster31

roadster31 Oct 5, 2015

Contributor

@bibich , for CSS and JS, is it possible to pass filters ?

Contributor

roadster31 commented Oct 5, 2015

@bibich , for CSS and JS, is it possible to pass filters ?

@bcbrr

This comment has been minimized.

Show comment
Hide comment
@bcbrr

bcbrr Oct 5, 2015

Contributor

For what it's worth I would also be in favour of keeping things simple, i.e. class methods for all hooks. I can understand the appeal of syntaxic sugar though, and Thelia is not only made for me :)

👍 for the hook cleaning commands in any case !

Contributor

bcbrr commented Oct 5, 2015

For what it's worth I would also be in favour of keeping things simple, i.e. class methods for all hooks. I can understand the appeal of syntaxic sugar though, and Thelia is not only made for me :)

👍 for the hook cleaning commands in any case !

@bibich

This comment has been minimized.

Show comment
Hide comment
@bibich

bibich Oct 5, 2015

Contributor

@roadster31 it's not yet possible to add filters for css or js but it's conceivable. We just have to find the right syntax.

Contributor

bibich commented Oct 5, 2015

@roadster31 it's not yet possible to add filters for css or js but it's conceivable. We just have to find the right syntax.

@nicolasleon

This comment has been minimized.

Show comment
Hide comment
@nicolasleon

nicolasleon Oct 7, 2015

Contributor

👍 As long as both methods are available.

Contributor

nicolasleon commented Oct 7, 2015

👍 As long as both methods are available.

gillesbourgeat added a commit that referenced this pull request Dec 3, 2015

@gillesbourgeat gillesbourgeat merged commit d89b79c into thelia:master Dec 3, 2015

2 checks passed

Scrutinizer 7 new issues, 22 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ThomasArnaud ThomasArnaud referenced this pull request Dec 9, 2015

Merged

Core module update #1788

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