[RFC] Support foo.some_thing === foo.someThing #934

simensen · 2012-12-13T16:27:40Z

I want to be able to support all underscores in my Twig templates. Most of my data in one of my projects comes from YAML files that are usually formatted like_this so it is a little jarring when in my template I need to mix with someMethod on an object:

paginated_items.theFirstItem.some_description

This PR allows for mapping the_first_item to theFirstItem.

paginated_items.the_first_item.some_description

I started a simple implementation of this. Happy to change it if there is a better way to go about it. I understand speed is a factor here so ideas on how I can minimize impact would be helpful.

The testing is pretty complicated. I did the bes tI could to try and figure out how to test this. Suggestions on how I can do that better would be helpful as well.

jmather · 2012-12-13T17:40:42Z

+1 I do this all the time and then have to go back and fix it, still.

You may also need to modify the pecl extension but I'm not 100% sure on that.

simensen · 2012-12-13T17:42:17Z

Yes, the pecl extension will need to be updated. It fails on travis. I didn't want to spend too much time on it without getting some feedback first. If this is going to get closed before getting off the ground I don't want to have dusted off my C hat for nothing. :)

stof · 2012-12-13T17:43:57Z

this is not consistent: you are doing it only for methods, not for array access or public properties

stof · 2012-12-13T17:45:36Z

btw, you should add some integration tests for it (by adding files in the test/Twig/Tests/Fixtures file, so that the consistency between the PHP and C implementations can be tested

stof · 2012-12-13T17:47:07Z

lib/Twig/Template.php

+        } elseif ($checkDecamelItem and isset(self::$cache[$class]['methods']['is'.$deunderscoreItem])) {
+            $method = 'is'.$deunderscoreItem;
+        } elseif ($checkDecamelItem and isset(self::$cache[$class]['methods']['__call'])) {
+            $method = $deunderscoreItem;


This is useless. If __call is defined, none of the deunderscoreItem cases can be reached

I pushed a change here. I moved the top __call block to be the last elseif block.

simensen · 2012-12-13T18:08:45Z

this is not consistent: you are doing it only for methods, not for array access or public properties

I will definitely hit array access and public properties if it makes sense to do those as well. From what I can see (looking at this point, anyway) there is less magic for those so I'm not sure what would be appropriate there.

btw, you should add some integration tests for it (by adding files in the test/Twig/Tests/Fixtures file, so that the consistency between the PHP and C implementations can be tested

Will do. I wanted to add one simple test to make sure it was working before I bothered sending out a call for feedback. I see that Travis is (correctly) failing because I haven't touched the C implementation yet.

Thanks for the feedback.

henrikbjorn · 2012-12-14T07:57:10Z

👎 You should proberly do a normalization step in your code instead.

igorw · 2012-12-14T15:59:10Z

In addition to what @henrikbjorn, it might be possible to pre-process your templates with a node visitor.

simensen · 2012-12-14T16:06:20Z

@henrikbjorn This is mostly about being able to provide a consistent feel in the templates where things are more heavily weighted on _'s instead of camelCase.

I feel like that is a big reason things are the way they are with the method translating as-is, so people can not have to worry about exactly how the backing data looks. For example, requesting foo.thatThing actually returns foo->getThatThing. Nobody is suggesting that I should normalize my data to do my own translation of getThatThing to thatThing.

@igorw If there is an easy and reusable way to do this with a node visitor I'm all ears. As you know I'm new to such things, so if you think it is possible and can help me a bit I'd be happy to try to work this out in that way.

mgenereu · 2013-02-13T02:25:13Z

I would love to help out on this. Every developer I know using Symfony and Twig thinks this is an inconsistency that it auto discovers the missing get to be more natural and not the case conversion from underscore. My "view guys" love twig and get confused when "variables following a dot" don't follow the rules for looking pretty.

Maybe it's an education issue but the big Symfony fans that I know are calling it a design bug in a system admired for its design and elegance.

simensen · 2013-02-13T17:05:32Z

@fabpot Would you be able to comment on this? I'd be happy to continue to work on this if there is a hope it might someday get merged. If you're a solid 👎 on it can we discuss it more and figure out if there is a way that I can do this that you would find acceptable?

There are at least a handful of people who have expressed that this would be useful to them and/or make life easier for themselves or their designers (aka "view guys") so I'd like to try and make this work if we can.

@igorw has offered to help talk to me about trying to handle this with node visitors and I'd be willing to try that and dig into the C source code once the PHP side of this PR is deemed acceptable.

fabpot · 2013-02-13T17:14:47Z

If we can come up with something that does not hurt performance, I'm 👍

vicb · 2013-02-13T19:58:56Z

I would love to see this "fixed", but I don't see how this would not impact perf (i once submitted a similar or which get rejected for that reason).

the Twig CS really don't help here as methods are camel cased while variables use "_". your Designer must know If he's dealing with a var or a method, one of the issue twig was supposed to solve !

mgenereu · 2013-02-13T22:20:33Z

Amen @vicb on "one of the issue twig was supposed to solve"! Also, for performance, if it is a later check in the case statement, the underscore removal check would only be unnecessarily ran if the method could not be found. That is an error screen and most people don't optimize error screens. The rest of the time, the earlier conditions would match. I think it's reasonable to add this on to the end of the checks with no performance hit other than when it's used.

Considering the uniqueness this is having and I commented this might be an education thing, here's a piece of code in one of my templates for everyone's reference. Maybe I'm not using Symfony correctly and that's why I'm feeling the ugliness. Thank you all for your time on this!

(Where Order has getSampleSets(), SampleSet has getIdCodes(), and IdCode has getFullScanName() )

{% for sample_set in order.samplesets %}
  <div>
    {{ sample_set.name }}
    {% for id_code in sample_set.idcodes %}
      <div>
        {{ id_code.fullscanname }}
      </div>
    {% endfor %}
  </div>
{% endfor %}

My front end people keep typing and I prefer:

{% for sample_set in order.sample_sets %}
  <div>
    {{ sample_set.name }}
    {% for id_code in sample_set.id_codes %}
      <div>
        {{ id_code.full_scan_name }}
      </div>
    {% endfor %}
  </div>
{% endfor %}

fabpot · 2013-02-13T22:56:19Z

As PSR-1 enforces camel case for method names ("Method names MUST be declared in camelCase"), the patch can be very simple: just strip the _ from the given item and only use this.

fabpot · 2013-02-13T22:57:38Z

My previous comment works well for method calls, but we also need to take care of properties and array calls; and as there are no standards for these, we must check both cases (which this PR does not address by the way).

vicb · 2013-02-14T02:48:00Z

@fabpot I am.not sure to get your point here: psr is not mandatory.

Fabien Potencier notifications@github.com wrote:

My previous comment works well for method calls, but we also need to
take care of properties and array calls; and as there are no standards
for these, we must check both cases (which this PR does not address by
the way).

Reply to this email directly or view it on GitHub:
https://github.com/fabpot/Twig/pull/934#issuecomment-13523606

mgenereu · 2013-03-08T21:27:30Z

So @simensen ... any ideas on getting this going? I don't even know what the PECL extension being referred to is.

mgenereu · 2013-03-11T17:45:13Z

@fabpot Does this need to work in both directions before it can be accepted? Can it work one way for now?

simensen · 2013-03-11T17:48:21Z

@mgenereu I haven't had time to follow up on this. It is probably going to take considerable work and I don't know when I'll be able to get it done. I haven't forgotten about it, though.

The PECL extension is a C implementation of Twig. You can see the code here: https://github.com/fabpot/Twig/tree/master/ext/twig

Even if I'm able to figure out a performant want to handle this problem in PHP I'll have to figure out how to do it in C as well.

I imagine that it has to work exactly the same for both C and PHP in order for this to be accepted, but @fabpot can weigh in on that.

mgenereu · 2013-03-11T18:07:21Z

An awesome programmer on my team has already implemented this for removing the underscores in PHP and C. @fabpot 's comment regarding properties and array calls means that it may not get accepted until other pure scenarios get addressed (hence my question above). I'm not completely clear on the examples for those as I was addressing a very specific Symfony awkwardness of "item -> getItem()" working and "first_item -> getFirstItem()" not working.

hason · 2014-01-16T13:34:06Z

The complete implementation is done in #1299. Please review.

mgenereu · 2014-01-19T01:26:21Z

Thank you for keeping this alive, @hason. Anything I can do to help or improve this?

RyanThompson · 2015-01-12T23:55:48Z

Looks like the PR about there - would LOVE to see this merged in. @vicb You are right PSR is not mandatory but standardized conventions make stuff like this a lot easier and reliable. We can assume the conventions we want to cover instead of a whole bag of them by adhering to standard rules ^_^

fabpot · 2015-01-25T12:14:35Z

Closing in favor of #1299

stof reviewed Dec 13, 2012
View reviewed changes

Support foo.some_thing === foo.someThing

15081be

hason mentioned this pull request Feb 4, 2013

Better mapping from lowercase to uppercase. #981

Closed

hason mentioned this pull request Dec 19, 2013

Added support for foo.some_thing === foo.someThing #1299

Open

fabpot force-pushed the master branch from dff2085 to 8d9a00d Compare October 22, 2014 14:39

Tobion mentioned this pull request Jan 12, 2015

Better variable accessor guessing #1590

Closed

fabpot closed this Jan 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Support foo.some_thing === foo.someThing #934

[RFC] Support foo.some_thing === foo.someThing #934

simensen commented Dec 13, 2012

jmather commented Dec 13, 2012

simensen commented Dec 13, 2012

stof commented Dec 13, 2012

stof commented Dec 13, 2012

stof Dec 13, 2012

simensen Dec 13, 2012

simensen commented Dec 13, 2012

henrikbjorn commented Dec 14, 2012

igorw commented Dec 14, 2012

simensen commented Dec 14, 2012

mgenereu commented Feb 13, 2013

simensen commented Feb 13, 2013

fabpot commented Feb 13, 2013

vicb commented Feb 13, 2013

mgenereu commented Feb 13, 2013

fabpot commented Feb 13, 2013

fabpot commented Feb 13, 2013

vicb commented Feb 14, 2013

mgenereu commented Mar 8, 2013

mgenereu commented Mar 11, 2013

simensen commented Mar 11, 2013

mgenereu commented Mar 11, 2013

hason commented Jan 16, 2014

mgenereu commented Jan 19, 2014

RyanThompson commented Jan 12, 2015

fabpot commented Jan 25, 2015

[RFC] Support foo.some_thing === foo.someThing #934

[RFC] Support foo.some_thing === foo.someThing #934

Conversation

simensen commented Dec 13, 2012

jmather commented Dec 13, 2012

simensen commented Dec 13, 2012

stof commented Dec 13, 2012

stof commented Dec 13, 2012

stof Dec 13, 2012

Choose a reason for hiding this comment

simensen Dec 13, 2012

Choose a reason for hiding this comment

simensen commented Dec 13, 2012

henrikbjorn commented Dec 14, 2012

igorw commented Dec 14, 2012

simensen commented Dec 14, 2012

mgenereu commented Feb 13, 2013

simensen commented Feb 13, 2013

fabpot commented Feb 13, 2013

vicb commented Feb 13, 2013

mgenereu commented Feb 13, 2013

fabpot commented Feb 13, 2013

fabpot commented Feb 13, 2013

vicb commented Feb 14, 2013

mgenereu commented Mar 8, 2013

mgenereu commented Mar 11, 2013

simensen commented Mar 11, 2013

mgenereu commented Mar 11, 2013

hason commented Jan 16, 2014

mgenereu commented Jan 19, 2014

RyanThompson commented Jan 12, 2015

fabpot commented Jan 25, 2015