Fixes regression added while fixing Bug #72740

[scara]

HI @0mars,
b1b44e0#diff-ab0834017b5c60ae382501bce9a32bbc added a breaking regression when using Query Fields qith no boost value due to a "NUL byte issue".
It has been there since 2.5 years ago: most of - i.e. not all - the package maintainers in several distro were still using 2.4.0 with just few patches for 7.2 compat.
Test coverage was unfortunately incomplete to highlight the #0; issue in the qf parameter.

2.5.0 is finally out and using it under the condition above breaks any expected response; the improved test now shows the issue:

# make test TESTS="--show-all tests/bug_72740.phpt"
[cut]
========DIFF========
016+     [qf] => score#0; manufacturedate_dt#0;
016-     [qf] => score^ manufacturedate_dt^
========DONE========

Long story short, I discovered this issue since months in a project called Moodle within the context of its CI Docker Toolbox: you can read more in moodlehq/moodle-php-apache#16 and moodlehq/moodle-php-apache#19 but limited spare time and the availability of workarounds kept me far from proposing kind of fix, waiting for the release of 2.5.0.

On 7/7 I found some spare time to explore the code and to propose a quick fix to highlight where the problem was (scara@c2c94b4) and posted it into https://bugs.php.net/bug.php?id=75631: @glensc was kind enough to do a quick peer review (really appreciated!).
I've decided to keep the initial quick fix in this PR since it looks like the way the code is actually managing the NULL byte check, even if @glensc is super right in suggesting a better refactoring which IMHO should be done at any level in the code to remove the pattern of checking for the value to be NULL and then via strlen() > 0 since it has been initialized with an empty string i.e. \0.

For the record, https://bugs.php.net/bug.php?id=72740 is still open even if it has been already "addressed".

HTH,
Matteo

[coveralls]

Coverage increased (+0.01%) to 79.64% when pulling 320fb73 on scara:master-fix-72740-regression into 3dbdb7e on php:master.

[scara]

Hi @glensc,
if you're fine w/ the current code I'd squash the commits into two: the first for the test changes and the latter for the code changes.

TIA,
Matteo

[glensc]

@scara LGTM; altho as expressed elsewhere, it would be cleaner if current_ptr->contents.arg_list.delimiter_override are not assigned empty values (value with length=0) but rather NULL. but this can be separate PR as there may be other values that need to be simplified like this.

[scara]

TNX @glensc

I agree w/ you:

1. the "bad thing" is using an empty string instead of NULL. Guessing some historical reason or an initial different code design
2. it would be nice to refactor the code, granted a good coverage of each refactored piece of code. Too big for this PR: besides, this extension is actually broken whenever the PHP code will use addQueryField() and this PR should be IMHO merged ASAP and 2.5.1 shipped

Will do the planned squashing late my evening.
Then, I'll open a specific issue describing how it would be useful such a refactoring and how it could be error-prone to stick with the empty string init pattern.

HTH,
Matteo

[scara]

Hi @glensc,
I've squashed and forced the push: PR is now clean TNX to your assistance 👍.

I've added a ref to this PR even in the original bug, https://bugs.php.net/bug.php?id=72740#1562881713.

HTH,
Matteo

[scara]

Hi @glensc,
is there something I can do to promote this PR?
Should I contact some package maintainers at least to share the issue in 2.5.0 and what workaround can be adopted to mitigate it?

TIA,
Matteo

[glensc]

@scara I'm nobody in pecl/php/pecl-solr scope, sorry ;) you need to reach maintainers somehow.

I can only encourage maintainers (with this comment) that this PR looks ok for me and should be merged into 2.5.1 patch release

[0mars]

Thanks a lot @scara @glensc, will follow up this weekend

[scara]

Hi @Jan-E,
I'm reading about your interest here from #13: is there anything I can do to help you in merging this PR?

[scara]

Still wondering why after 7 months the PR is still at this stage, without a peer review from the maintainers.

BTW TNX to @glensc for the time spent on helping me, really appreciated!

Matteo

[0mars]

Thanks

[stronk7]

Hi, is there any scheduled planned for this, say 2.6.0 or 2.5.1 or anything?

Having to build solr everywhere with the patch applied, because the upstream version simply doesn't work ok, or having to force boost values as a workaround aren't really the best options.

That said by an ignorant that knows nothing about the internals, just the experience is suboptimal, I'm afraid.

Many thanks for all the work with this awesome extension, much appreciated!

Ciao :-)

[0mars]

@stronk7 @scara yes, there is a scheduled release now, 2.5.1, expected by the end of July.

Apologies for being away. I have been very busy, still am but now I will continue the development gradually!

[php-pulls]

Comment on behalf of omars at php.net:

merged