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

Allow arrays (Hash::flatten'ed) to be used in substitutions #6956

Merged
merged 1 commit into from Jul 9, 2015

Conversation

hmic
Copy link
Contributor

@hmic hmic commented Jul 6, 2015

This allows for the standard . notation of arrays to be used within substitions like
attendee.fistname and event.name from e.g. Hash::flatten($entity->toArray());

As the . notation is used in many places, this would make sense here too.

@ADmad
Copy link
Member

ADmad commented Jul 6, 2015

I also wanted to be able to use template vars with - in name :)

@ADmad ADmad added this to the 3.0.9 milestone Jul 6, 2015
@hmic
Copy link
Contributor Author

hmic commented Jul 6, 2015

So maybe just allow all valid variable names?

@markstory
Copy link
Member

- isn't valid in PHP variable names 😄

@hmic Having a test for this would be great.

@ADmad
Copy link
Member

ADmad commented Jul 7, 2015

@markstory I never said my wants were valid 😛

@hmic
Copy link
Contributor Author

hmic commented Jul 7, 2015

So which way to go?

  • Allow valid php variable names?
  • Allow all valid array indices?
  • Allow all but space, curly braces and quotes?
  • Add . and - to the list and stick with word characters otherwise, localized or ascii only?

@ADmad
Copy link
Member

ADmad commented Jul 7, 2015

Replacement data is provided as associative array with keys used as template variable name. So I guess any valid array key should be allowed.

@hmic
Copy link
Contributor Author

hmic commented Jul 7, 2015

So this is about everything (integer or (cast to) string), except arrays or objects... On one hand I like the idea and it would be the "correct" solution as you said, on the other hand it's kind of... hmm...

@ADmad
Copy link
Member

ADmad commented Jul 7, 2015

So this is about everything (integer or (cast to) string), except arrays or objects...

Not sure what you mean :)

@markstory
Copy link
Member

I think accepting any valid array key (which is pretty much anything) will have some problems as }} is totally legal in an array key. Sticking with simpler word characters and . will solve the current problem and let us expand functionality in the future as well.

@hmic hmic force-pushed the patch-5 branch 2 times, most recently from ae5cd3c to d3a99dc Compare July 7, 2015 11:07
@ADmad
Copy link
Member

ADmad commented Jul 7, 2015

I agree with @markstory. Lets keep it simple world list with dot and dash :troll:.

@hmic
Copy link
Contributor Author

hmic commented Jul 7, 2015

Thats what my last commit does, characters, numbers, space and .-_

But as I amended mylast one, I think travis did not fetch the new commit :-(

Should I remove space, numbers and _ and push again or is this sane enough?

@ADmad
Copy link
Member

ADmad commented Jul 7, 2015

@hmic Travis will fetch whatever you push to your branch. If you amended your last commit instead of making a new one you will have to force push your branch to github.

@dereuromark
Copy link
Member

Tests still needed, though.

@hmic hmic force-pushed the patch-5 branch 4 times, most recently from 427212a to f39a440 Compare July 7, 2015 13:41
@markstory
Copy link
Member

I wouldn't include -. - is a common operator. If in the future we wanted to expand the mini-language and allow math operations, we would not be able to do that.

@ADmad
Copy link
Member

ADmad commented Jul 7, 2015

😿

@hmic
Copy link
Contributor Author

hmic commented Jul 8, 2015

  • has been requested, so there it is!
    My original PR was about adding the . only. This would not allow for (floating point) math operations too.

In my oppinion it's far more usefull to be able to (at least) use flattened array data than doing math operations (in the future).

On the other hand, it would be easy to allow other oparations on the substitutions with extra substitution classes like [[ ]] for math: [[ {{entity.my-var}} - {{any_other thing}} ]]

If thats a too big deal, revert to only additionally allow for the dot like proposed originally?

@markstory
Copy link
Member

@hmic Lets add . for now and worry about other values later.

@hmic
Copy link
Contributor Author

hmic commented Jul 8, 2015

Numbers are considered ok too? Maybe _?

I really like to be able to use fields of my database tables from flattened arrays...

E.g. arrays (Hash::flatten'ed) to be used in substitutions.

This allows for the standard . notation of arrays to be used within substitions like
attendee.fistname and event.name from e.g. Hash::flatten($entity->toArray());
@lorenzo
Copy link
Member

lorenzo commented Jul 9, 2015

Thanks @hmic !

lorenzo added a commit that referenced this pull request Jul 9, 2015
Allow arrays (Hash::flatten'ed) to be used in substitutions
@lorenzo lorenzo merged commit 7e89611 into cakephp:master Jul 9, 2015
@markstory
Copy link
Member

Looks great now.

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

Successfully merging this pull request may close these issues.

None yet

5 participants