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

[WIP] Added Templating Helper for PHP templates #62

Merged
merged 3 commits into from
Jun 30, 2013

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jun 25, 2013

fixes #61

* Get the extension name
*
* @return string
*/
public function getName()
{
return 'cmf_extension';
return 'cmf';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is a correct name, but I don't think cmf_extension is the correct name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it maybe be cmf_core ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah well, I think I prefer cmf then. As 'core' doesn't add much information and core is just another name for 'default things' which is also what 'cmf' only means.

@dbu
Copy link
Member

dbu commented Jun 25, 2013

seems good to me. is the name of the twig extension important? i mean, does anything rely on that?

this PR will collide with #59 but i will manage to fix that :-) lets merge this one first.

@dbu
Copy link
Member

dbu commented Jun 25, 2013

btw, we started requiring the symfony PR header - please use it when creating PR to make them more clear

@wouterj
Copy link
Member Author

wouterj commented Jun 25, 2013

The getName function for the Twig Extension is not relevant, as long as other filters and functions aren't using it. (you can get the extension by calling $context->getEnvironment()->get('extension name') inside another extension).

But the name is important in PHP templates, as that's the only way to get the helper:

<a ...><?php $view['extension_name']->getPrev(); ?></a>

@dbu
Copy link
Member

dbu commented Jun 25, 2013

but should we not use cmf_core then, to follow the principels of least
surprise? the other extension is cmf_block. though we could argue for
just cmf here too, dunno. as we are core, i don't fear name collisions.
you fixed it so you get to pick the name :-)

@wouterj
Copy link
Member Author

wouterj commented Jun 25, 2013

Hmm, all functions has something to do with documents, so we can call it documents. That somehow follows how the frameworkbundle handles templating helpers, the name should not be explicitely related to the bundle name. But that means cmf_block should be blocks and that causes some inconsistency with sonata_block (which also should be blocks).

What about overriding (if possible) the sonata block helper with the cmf helper (which also provides the methods of the sonata helper. When we combine the sonata bundles with the cmf bundles, we can make this a lot easier and just have one helper class for the blocks.

tl;dr

  • I think blocks and documents are the most logic names for PHP template users, as they are already used to use the slots and actions helpers.
  • To integrate this into the cmf, we can extend SonataBlockHelper from CmfBlockHelper (which we will rename to BlocksHelper). We register the BlocksHelper as the blocks templating helper and advocate the use of that helper instead of the sonata block helper, to be feature compatible when we merge the sonata bundles.
  • We call this helper documents

wdyt?

@dbu
Copy link
Member

dbu commented Jun 25, 2013

i agree with extending the sonata block helper and overriding it to provide one combined helper, that sounds logical.

calling this one documents however smells like too risky of name clashes to me. afaict there is no way to change the names (unless you extend the class to overwrite the method, and make the setup somehow pick the right class) so i am quite a bit afraid of name clashes.

@dbu
Copy link
Member

dbu commented Jun 27, 2013

and documents also has the drawback that in the long run this probably will be about "entities" too. i'll try to get some input from the mailinglist.

@wjzijderveld
Copy link

Considering the slots and actions, I think blocks makes sense.

Not sure about the documents as well, I think that you shouldn't make it about documents in your templates, but about content (in whatever way it may be stored).
So content crossed my mind, but that might be to generic again...

And again we can find this phrase to be correct :)
There are only two hard things in Computer Science: cache invalidation and naming things.

@dbu
Copy link
Member

dbu commented Jun 27, 2013

i thought the phrase goes |There are only two hard things in Computer
Science: cache invalidation, naming things and off by one errors.|

what if we would just call the extension "cmf" instead of "document" or
"content"?

@wjzijderveld
Copy link

Works for me as well, it's just a bit less descriptive, but in the end that is what it is.
It would leave the option open to add stuff to that extension not directly related to content/documents.

@wouterj
Copy link
Member Author

wouterj commented Jun 27, 2013

If the stuff is not related to content/documents, it should go in another templating helper. If you look at the templating helpers in the FrameworkBundle you can see a lot of helpers. Helpers aren't related to a bundle, but to a feature. That's why I also think cmf_core isn't a correct name.

cmf is the name I already used in this PR, I can live with it :)

@dbu
Copy link
Member

dbu commented Jun 27, 2013

ok, if we say we keep cmf as name, is this ready to merge?

@dbu
Copy link
Member

dbu commented Jun 30, 2013

ping

@wouterj
Copy link
Member Author

wouterj commented Jun 30, 2013

Yes, it's mergable afaics

(actually, I don't know if the tests pass, you forgot to register the travis hook?)

dbu added a commit that referenced this pull request Jun 30, 2013
[WIP] Added Templating Helper for PHP templates
@dbu dbu merged commit 47238bb into symfony-cmf:master Jun 30, 2013
@dbu
Copy link
Member

dbu commented Jun 30, 2013

oh btw, do you also do a doc PR about php templating?

@wouterj
Copy link
Member Author

wouterj commented Jun 30, 2013

Yes

@lsmith77
Copy link
Member

lsmith77 commented Jul 5, 2013

the service configuration also needed to be updated 25f3955

@dbu
Copy link
Member

dbu commented Jul 5, 2013

ups, thanks. do you think we can activate travis on PRs here?

@lsmith77
Copy link
Member

lsmith77 commented Jul 5, 2013

its on for everybody
http://about.travis-ci.org/blog/pull-request-testing-for-everyone/

.. but sometimes there are simply delays

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.

provide php templating functions as well
4 participants