search

DPDBeNeLux / magento-DPD_Shipping

Let op: er zijn nieuwe DPD plugins (BETA versie) beschikbaar met belangrijke nieuwe functionaliteiten. De oude plugins zullen na Brexit niet meer goed functioneren, download daarom de nieuwe versie op Github/DPDconnect.
11 stars 10 forks source link

Improved regex for shipping method label detection #47

Open vovayatsyuk opened 8 years ago

vovayatsyuk commented 8 years ago

This change allows to use multiple attributes inside label tag, to improve module stability, when some third-party modules override shipping methods template.

See http://www.phpliveregex.com/p/el7

mvgucht commented 8 years ago

Hey,

thanks for the proposition. I know that in the original regex the (.?) is used to. but it seems redundant. . stands for zero or more characters, so why make it a conditional capturing group.

preg_match('!<label(.*?)for="(.*?)parcelshops"(.*?)>(.*?)<\/label>!s', $html, $matches);

I think this could be rewritten as:

preg_match('!<label.*for=".*parcelshops".*>.*<\/label>!s', $html, $matches);

this will result in a single element array $matches when it is matched.

what do you think?

vovayatsyuk commented 8 years ago

I agree with you.

However, I removed capturing groups added by me, and left groups that was added by module developers.

If module developers will agree too, I will add additional commit to this PR.

vovayatsyuk commented 8 years ago

Oops, just saw that you are the module developer :)

So I think I should remove all groups and ? too?

mvgucht commented 8 years ago

I'm not the original developer :) but I've worked on it quite a bit in the last year. i'll test the new commit as soon as possible and merge it with the master if I can't find any issues.

hostep commented 8 years ago

Hi guys

I would keep the ?'s in the regex, otherwise it is possible it will match multiple <label>'s at once. See the example at: http://www.regexpal.com/?fam=94132, by default it has 2 matches, but if you remove the question marks, it will match the two <label>'s as one big match, which would be incorrect.

The ? operand determines how greedy the regex is, if you don't specify it, it will try to match as much as possible, whereas if you use ? it will try to match as little as possible.

So I vote for keeping the ? in the regex.

mvgucht commented 8 years ago

you are right, but what if we use the global 'U' parameter like this:

https://regex101.com/r/aS4tG0/1

hostep commented 8 years ago

Ah nice, didn't knew that modifier existed. That's also fine with me :)