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

Fix xpath-expressions (#35) #39

Merged
merged 5 commits into from
Jul 24, 2018
Merged

Fix xpath-expressions (#35) #39

merged 5 commits into from
Jul 24, 2018

Conversation

twiro
Copy link
Member

@twiro twiro commented Feb 5, 2018

This change aims to implement support for all valid xpath-expressions inside the „expression“-field.

See #35 for details.

This change aims to implement support for ALL valid xpath-expressions
inside the „expression“-field.

See #35 for
details.
@@ -385,9 +385,38 @@ public function compile(&$entry)
// Find queries:
preg_match_all('/\{[^\}]+\}/', $expression, $matches);

// Find replacements:
// Get root node name
$root_node_name = $xpath->evaluate('name(/*)');
Copy link
Member

Choose a reason for hiding this comment

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

I would do

preg_quote ((string)$xpath->evaluate('name(/*)'), '#');

@twiro
Copy link
Member Author

twiro commented May 31, 2018

I added the requested change and ran all tests again – looking good!

It would be great if this could go into the integration-branch, I'd then prepare the readme and the meta and submit another PR for a new 2.0.4-release that should fix all the irritation about expressions!


// Insert a '/' if the expression contains a '(' directly followed by the root node name
// Fixes expression 1.J (when used inside an xpath-function)
else if (strpos($match,'('.$root_node_name) !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry about that, we need to keep $root_node_name intact to make the strpos work... !!

$root_node_name = (string)$xpath->evaluate('name(/*)')
$root_node_name_quoted = preg_quote ($root_node_name, '#'); // <-- use in preg_* calls only

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented it as suggested – but as I'm really not an expert regarding all this preg/regex-stuff, I would be glad if you could give me a hint what kind of difference exists between my previous solution…

preg_match('#^{'.$root_node_name.'#', $match))

and the one we have now:

preg_match('#^{'.$root_node_name_quoted.'#', $match))

I mean both work as expected in all situations I tested, but I'd really like to understand that change :)

@twiro
Copy link
Member Author

twiro commented Jun 2, 2018

So now this should be ready to go into 'integration' :)

I will prepare the readme and meta for the 2.0.4-release and send another PR – or would you mind giving me direct access to this repo so I can manage the release (and maybe also an additional gh-pages-branch for the docs) myself?

+ Better solution for modifying path-expressions (#35)
+ Fixing some indenting…
@twiro
Copy link
Member Author

twiro commented Jun 3, 2018

I improved my solution so I have to call the xpath-evaluation only once and also will work without preg-calls.

I also added "backwards-compatibilty" with "wrong" xpath-expressions that worked in version 1.X of the extension.

Tested this again all use cases, looking good and I hope these are my final changes to this commit ;)

@nitriques nitriques merged commit 3cbe0aa into symphonists:integration Jul 24, 2018
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.

2 participants