Stricter/clearer rules and documentation for expressions

[twiro]

After having faced the already mentioned troubles with XSLT utilites in version 2.0.2 I've set up a small comparison chart to see how different expressions react in different versions of the extension.

And while the last update fixes a bug (2.A) that was introduced with version 2.0 it also breaks the recommended way this extension is supposed to work - the method described by the inline-help under the expression-field (1.D) does not work anymore in version 2.0.2:

"To access the other fields, use XPath: {entry/field-one} static text {entry/field-two}. "

Thinking further this help-text seems to be a relict of version 1.X as it doesn't proper reflect the changes to the XML-structure introduced with version 2.0. I don't know if by chance or by intention, but this method worked in versions 2.0.0 and 2.0.1 though obviously the proper XPath in the expression-field should look like this since version 2.0:

"...use XPath: {reflection-field-handle/entry/field-one} static text {reflection-field-handle/entry/field-two}. "

This looking way more irritating and verbose (2 variables that need to be replaced and thought of instead of one) than the upper solution is one of the reasons I'm not a huge friend of the updated XML-structure, but the Xpath-example in the help-text could also be changed to this for easier reading/understanding:

"...use XPath: {//entry/field-one} static text {//entry/field-two}. "

Second thing that is broken in 2.0.2 and actually should work is simply using the root node of XML-Data generated by an XSLT-utility (2.B).

Some other methods that (by chance or by intention) were introduced with version 2.0 were 'broken' by 2.0.2 also, but these methods don't look like valid XPath-expressions given the current XML-structure, so removing them might by a valid decision (though it probably shouldn't happen in point-releases).

Reflection Field & XPath Expressions

LEGEND

✅ = Returning a valid value (AND expected to do so)
✔️ = Returning a valid value (though NOT expected to do so)
➖ = Returning empty/undefined (as expected)
✖️: = Returning empty/undefined (though the help-text says it should work)
❌ = Returning empty/undefined (though expected to work)
❗ = Throwing a system error (though expected to work)

---

1) Reflecting entry data directly in the expression

The table shows how the different expressions process the value of a field named 'title':

	Expression	1.4.2	2.0.0	2.0.1	2.0.2	2.0.3
1.A	{title}	➖	✔️	✔️	➖	➖
1.B	{/title}	➖	✔️	✔️	➖	➖
1.C	{//title}	✅	✅	✅	✅	✅
1.D	{entry/title}	✅	✔️	✔️	✖️	➖
1.E	{/entry/title}	➖	✔️	✔️	➖	➖
1.F	{//entry/title}	✅	✅	✅	✅	✅
1.G	{reflection-field-handle/entry/title}	➖	✔️	✔️	✔️	✔️
1.H	{/reflection-field-handle/entry/title}	➖	✔️	✔️	➖	➖
1.I	{//reflection-field-handle/entry/title}	➖	✅	✅	✅	✅
1.J	{data/reflection-field-handle/entry/title}	➖	✅	✅	❌	❌
1.K	{/data/reflection-field-handle/entry/title}	➖	✅	✅	✅	✅
1.L	{//data/reflection-field-handle/entry/title}	➖	✅	✅	✅	✅
1.M	{substring(//title, 3)}	✅	❗	❗	✅	✅

---

2) Reflecting entry data with the help of a XSLT utility

The table shows how the different expressions process the following example data (returned by a XSLT utility):

<reflection>Value</reflection>

	Expression	1.4.2	2.0.0	2.0.1	2.0.2	2.0.3
2.A	{/}	✅	❗	❗	❌	❌
2.B	{reflection}	❌	✅	✅	❌	❌
2.C	{/reflection}	✅	✅	✅	✅	✅
2.D	{//reflection}	✅	✅	✅	✅	✅

---

Proposal:

I assume this extension plays a quite important role in lots of projects and the way it is working shouldn't change all too often (especially not in simple point-releases). So I'd suggest we simply agree upon which kind of expression version 2.X of this extension should 'officially' support (only valid XPaths? Everything that was introduced in 2.0?), update the help-text and the documentation accordingly (one or two examples regarding expressions wouldn't be bad) and release a proper working version 2.1 soon.

Therefore it would be great to collect some opinions about how expressions are currently used (and expected to work) from different developers.

I'll gladly help out with documentation (after we agreed upon the desired behavior regarding expressions), but it would be great if someone more involved in the extension's logic ( @nilshoerrmann or @nitriques ) could have a look at fixing the current xslt-utility-bug.

[nitriques]

could have a look at fixing the current xslt-utility-bug.

I am still waiting for confirmation on the fact that it may only be a xpath issue...

Thanks for this effort @twiro.

Adding the hardcoded // (which made the {title} syntax worked) is not ok, since it prevents calling a function (like substring(x, 3)) in the Xpath expression.

I do not use this field often, so I can't judge on what usage is good or not. I trusted @nilshoerrmann for his changes that seems reasonable and good.

While the removal of the '//' looked like a bugfix to me, I now understand that it was a breaking change.

And by looking at your results, 2.0.2 seems fine. We would need to update the inline doc tho.

[nitriques]

I other words, the loss of the {title} syntax for more flexibility is a good move. But is it a breacking change, not a bug fix.

[nitriques]

@twiro We've received feedback for #34 and nothing it 'ignored' (it yields the same results as your tests). So please send a PR with the doc fixed, and we'll release version 3.0.0, to make sure we advertise our stuff correctly. I would also like it if you could add your test result in the README. Thanks again for you hard work.

[twiro]

Adding the hardcoded // (which made the {title} syntax worked) is not ok, since it prevents calling a function (like substring(x, 3)) in the Xpath expression. [...] In other words, the loss of the {title} syntax for more flexibility is a good move. But is it a breacking change, not a bug fix.

I agree! The hardcoded // is not a good solution and should be removed in 3.0.0.

And by looking at your results, 2.0.2 seems fine.

I wouldn't call it fine. If we go for a new major release I'd aim for making all valid xpath-expressions work in the expression-field.

Most things work as expected in 2.0.2 but 1.J and 2.B (From my comparison list above) don't. In more precise words: Reflection field currently can't process expressions that select the root node directly without a leading / or //.

I spent the last hour having a look at the code and trying to understand what's going on, but obviously my knowledge of XPath/XML in a PHP-surrounding is too limitited - so I'm a little lost here. @nitriques - could you have a look? I think this should really be fixed before we release a new major version.

There are two other minor issues left, but I don't think they are much of a problem:

1.D) {entry/title}

This syntax is not working in 2.0.2 which is fine, as it's not a valid xpath. The only problem is that it's still the recommended syntax in the inline-help. I'd suggest to go with {//entry/title} instead. If we can agree upon this I'd gladfully take care about updating the documentation.

1.G) {reflection-field-handle/entry/title}

This syntax is working in 2.0.2 though it's not supposed to (no valid xpath).
So it's not a real problem I guess - but I wouldn't include it in the documentation/examples nor promote it anywhere else.

I would also like it if you could add your test result in the README. Thanks again for you hard work.

I'd suggest to only include a subset of recommended (and working) expressions, but maybe put the full comparison list in the wiki and link to where from the readme. One reason for that would be to keep the readme clean and focussed, the other one that all this fancy tables- and github-emoji-stuff won't work on symphonyextensions.com!

[nitriques]

Reflection field currently can't process expressions that select the root node directly without a leading / or //.

Yup. That's PER DESIGN. Older version would simply hardcoded the // to make it work.

1.D) {entry/title} ... If we can agree upon this I'd gladfully take care about updating the documentation.

We do agree! It is NOT valid!

1.G) {reflection-field-handle/entry/title} ... though it's not supposed to (no valid xpath).

It is valid xpath. The context is /, so you can omit it if you are selecting first level elements.

but I wouldn't include it in the documentation/examples nor promote it anywhere else.

Agreed. We can keep it in the wiki tho.

One reason for that would be to keep the readme clean and focussed, the other one that all this fancy tables- and github-emoji-stuff won't work on symphonyextensions.com!

Agreed!

If you still want to send a PR, that would be wonderful. It should not involder I'll create the release, just update the docs and create the wiki (make add a link in the readme to the wiki ?)

[twiro]

Yup. That's PER DESIGN. Older version would simply hardcoded the // to make it work.

Mmh. So you're saying we should simply sell this as intended behavior? We could do so (if it's well documented) but from a user-perspective I find it hard to understand why simply writing {root} or {root/section/entry} should not work as these look like perfectly valid xpath-expressions to me.

So by "PER DESIGN" you mean it's too hard to make it work or are you saying you think it should work the way it does right now?

1.G) {reflection-field-handle/entry/title} ... though it's not supposed to (no valid xpath).

It is valid xpath. The context is /, so you can omit it if you are selecting first level elements.

But isn't data the first level element? And therefore {data/reflection-field-handle/entry/title} should work without the leading / (which it doesn't!) instead of {reflection-field-handle/entry/title} which (as far as I understand) should need a leading // to work?

If I match / in my master-template-in Symphony I still have to select data/section/entry inside my template to proceed to a specific entry. Simply selecting section/entry does not work. I would have to write //section/entry instead. So isn't this the exact same scenario as the one outlined above and shouldn't it work the same way in reflection field?

In my eyes the XPath in the expression-field should work exactly the same way as it does in a tool like this: http://www.xpathtester.com/xpath/8cc934fa60ca9778eaa492a4bdcdc617

Right now it doesn't!

I'm a little finicky here because I like @nilshoerrmann's main intention behind 2.0.0 (make reflection field more conform to the frontend regarding XML-structure and XPath/Templating) and I'd really like to simply say "all valid xpath-expressions are ok" in the docs rather than having to outline unexpected exceptions without even beeing able to explain why these scenarios don't work.

[nitriques]

So by "PER DESIGN"

I think it should work the way it does right now, yes! Having the possibility to call xpath function is a good thing IMHO.

But isn't data the first level element? And therefore...

Yup that's weird. Even weirder, we are usign php's DOMXPath object, not our XsltProcessor class, so that might explain the differences.

and I'd really like to simply say "all valid xpath-expressions are ok"

I think we would need to change from using string() to a proper value-of to get the same results.

[nitriques]

1.D has been fixed in 4500a3a so we are left with 1.G and the fact that it does not uses the XSLTProcessor class.

[twiro]

I just submitted a PR that includes a few exceptions regarding the xpath-evaluation so that all abovementioned valid xpath-expressions now work as expected. I tested it thoroughly but more testing wouldn't be bad ;) Maybe @animaux or @jurajkapsz want to jump in as they already dealt with the problems this PR is trying to fix. Would be very much appreciated!

Before publishing this in a new release I'd like to update the expression-compatibility-table in this thread, move it to the wiki of this repo and extend the readme by linking to the information in the wiki.

@nitriques – could you give me write-access to this repo so I could take care about that?

---

Reflection Field & XPath Expressions

LEGEND

✅ = Returning a valid value (AND expected to do so)
✔️ = Returning a valid value (though NOT expected to do so)
➖ = Returning empty/undefined (as expected)
✖️: = Returning empty/undefined (though the help-text says it should work)
❌ = Returning empty/undefined (though expected to work)
❗ = Throwing a system error (though expected to work)

1) Reflecting entry data directly in the expression

The table shows how the different expressions process the value of a field named 'title':

	Expression	2.0.0	2.0.1	2.0.2	2.0.3	2.0.4
1.A	{title}	✔️	✔️	➖	➖	➖
1.B	{/title}	✔️	✔️	➖	➖	➖
1.C	{//title}	✅	✅	✅	✅	✅
1.D	{entry/title}	✔️	✔️	✖️	➖	✔️
1.E	{/entry/title}	✔️	✔️	➖	➖	✔️
1.F	{//entry/title}	✅	✅	✅	✅	✅
1.G	{reflection-field-handle/entry/title}	✔️	✔️	✔️	✔️	✔️
1.H	{/reflection-field-handle/entry/title}	✔️	✔️	➖	➖	➖
1.I	{//reflection-field-handle/entry/title}	✅	✅	✅	✅	✅
1.J	{data/reflection-field-handle/entry/title}	✅	✅	❌	❌	✅
1.K	{/data/reflection-field-handle/entry/title}	✅	✅	✅	✅	✅
1.L	{//data/reflection-field-handle/entry/title}	✅	✅	✅	✅	✅
1.M	{substring(//title, 3)}	❗	❗	✅	✅	✅

---

2) Reflecting entry data with the help of a XSLT utility

The table shows how the different expressions process the following example data (returned by a XSLT utility):

<reflection>Value</reflection>

	Expression	2.0.0	2.0.1	2.0.2	2.0.3	2.0.4
2.A	{/}	❗	❗	❌	❌	✅
2.B	{reflection}	✅	✅	❌	❌	✅
2.C	{/reflection}	✅	✅	✅	✅	✅
2.D	{//reflection}	✅	✅	✅	✅	✅
2.E	{substring(/, 3)}	❔	❔	❔	❔	✅
2.G	{substring(reflection, 3)}	❔	❔	❔	❔	✅
2.G	{substring(/reflection, 3)}	❔	❔	❔	❔	✅
2.H	{substring(//reflection, 3)}	❔	❔	❔	❔	✅

---

[jurajkapsz]

Can do next week. Some types of use might be new to me :) but I can give it a round.

[nitriques]

Amazing work @twiro Thanks.

@nitriques – could you give me write-access to this repo so I could take care about that?

I would prefer if you would add this info to the README file, or another file directly in the repo. Dealing with the wiki is a pain and want to get rid of them. Tying the doc and the code is the way to go.

I'll wait before merging, please go test ahead ! ping me if needed.

[twiro]

Time to get this finished! I submitted the changes you requested to my PR and ran the tests again and guess it's ready to go into integration (though more testing still would be appreciated).

I would prefer if you would add this info to the README file, or another file directly in the repo.

I don't wan't to put tables (or other "advanced formatting") into the readme as symphonyextensions.com doesn't get along nicely with most of this (the current readme serves as a perfect example by the way).

_ Dealing with the wiki is a pain and want to get rid of them._

Ok… so no Wiki!

Tying the doc and the code is the way to go.

Sounds like setting up a separate branch and using github pages for deeper documentation and/or examples would be the best solution right now… correct?

Already went this route with my multilingual-extension and while I still need to finish and polish things I think it's a pretty good solution if the readme contains links to everything happening there!

Anyway – after submitting this PR to integration I'd like to fix some things in the readme, so it would be great if you'd wait for another PR before this goes into the official 2.0.4-release.

[nitriques]

Sounds like setting up a separate branch and using github pages for deeper documentation and/or examples would be the best solution right now… correct?

Yup!

Anyway – after submitting this PR to integration I'd like to fix some things in the readme, s

So it is ready to merge ?

[twiro]

Sounds like setting up a separate branch and using github pages for deeper documentation and/or examples would be the best solution right now… correct?

Yup!

Alright :)

Or do you think a simple "docs"-folder containing one or more md-files would be a better solution for additional extension documentation? Whatever we agree on, we could also put it into https://github.com/symphonists/symphony-extensions-network under "best practices" – would be good to have a "standard way" of doing this across extension-repos!

So it is ready to merge ?

Yes! PR39 and PR41 should do the job for now! I simply linked the readme to the comparison chart in this thread and will transform all the information into proper documentation later…