Skip to content
This repository was archived by the owner on Feb 1, 2019. It is now read-only.

Convert hyphens to underscores for jinja variable syntax#7

Merged
Scotchester merged 2 commits intocfpb:masterfrom
kurtrwall:attachment
Apr 28, 2015
Merged

Convert hyphens to underscores for jinja variable syntax#7
Scotchester merged 2 commits intocfpb:masterfrom
kurtrwall:attachment

Conversation

@kurtrwall
Copy link
Copy Markdown

Any data that needs to be accessed in jinja2 is through a named variable that holds that data. The naming syntax of jinja2 does not allow hyphens. This takes any variable named with a hyphen and converts the hyphen into underscores.

@dpford @Scotchester @willbarton

@Scotchester
Copy link
Copy Markdown

I think we should probably do this, but I'm curious, what currently happens if there's a hyphenated name in the JSON output? We simply can't access it in Jinja?

@kurtrwall
Copy link
Copy Markdown
Author

That's right. To test, try to replace an image with the consumer-stories image size. It should be something like post.thumbnail.images.consumer-stories.url. It won't work.

@Scotchester
Copy link
Copy Markdown

So this change shouldn't break anything already in place, correct?

@kurtrwall
Copy link
Copy Markdown
Author

No because it's only looking for hyphens and replacing those with underscores. The only code that accesses those variables by the hyphened name doesn't access it through the wp-json-api plugin so we're safe on that front.

@Scotchester
Copy link
Copy Markdown

👍 I just noticed that the README includes a line stating the current stable version. This seems unnecessary, given the version number in the main PHP file and the tags/releases. Can you please remove that README line in this PR?

Let's make this and #5 (when that gets updated to use maybe_unserialize()) 1.1.5.

@Scotchester
Copy link
Copy Markdown

Just ran into a place where this would be useful. Gonna go ahead and merge so I can update it in my UnityBox WordPress.

Scotchester added a commit that referenced this pull request Apr 28, 2015
Convert hyphens to underscores for jinja variable syntax
@Scotchester Scotchester merged commit 9cb5511 into cfpb:master Apr 28, 2015
@kurtrwall kurtrwall deleted the attachment branch April 28, 2015 17:38
@Scotchester
Copy link
Copy Markdown

This only applies to images? We need this to apply to all custom fields.

@Scotchester
Copy link
Copy Markdown

Also, any particular reason why str_replace wouldn't work?

@KimberlyMunoz
Copy link
Copy Markdown

This may be silly, but would using brackets instead of dot notation be helpful here?

images.consumer-stories.url -> images['consumer-stories']['url']

@Scotchester
Copy link
Copy Markdown

I think that works, but I do love the simplicity of the dot notation...

Are you implicitly arguing for not forcibly converting them? I can see the argument for that. "Either don't use them when you define your keys, or deal with bracket notation."

@Scotchester
Copy link
Copy Markdown

P.S.: I have a working modification to fix the custom field names running locally.

@KimberlyMunoz
Copy link
Copy Markdown

Actually, I didn't know if there was a reason why we couldn't use bracket notation that I hadn't seen yet.

I'm a little hesitant about forcibly converting, especially in cases where something like contact-us and contact_us could both exist and cause an error that would be difficult to debug later. But if it makes things easier for developers and we have a check for similar slugs, it's probably fine and I don't have any ideological objections to it.

@Scotchester
Copy link
Copy Markdown

Could probably use opinions from other FEWDs that have worked with WordPress data in Sheer. /cc @virginiacc @cfarm @jimmynotjim @sebworks @anselmbradford @himedlooff

@kurtrwall
Copy link
Copy Markdown
Author

@KimberlyMunoz makes a good point about the bracket notation, though I share the same sentiment as @Scotchester about the dot notation.

To be clear, this PR came out of a choice in Wordpress to use the post-thumbnail built in image size. The name could not be changed so I created this PR to deal with it. Now that we aren't using that built in image size name, there is no direct need for it from me at this point.

str_replace would work just fine in this case, and it should probably be used instead.

@Scotchester
Copy link
Copy Markdown

I guess I'm coming around to not doing this. People can either avoid hyphens, or deal with the bracket notation.

@kurtrwall
Copy link
Copy Markdown
Author

Do we want to revert the PR then?

@Scotchester
Copy link
Copy Markdown

Yeah, I think we do. @KimberlyMunoz, what do you think?

@KimberlyMunoz
Copy link
Copy Markdown

I think so. If we're going to revert, better to do it quick before things get built on it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants