[5.x.x] Fixes to fn:replace, fn:tokenize, and fn:analyze-string

[adamretter]

Backport of #4864

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

[line-o]

Changes like this were considered breaking in the past.
If in any application running on exist (a future version of 5) calls fn:matches((1,2), "\d") will now throw an error.

[line-o]

I just want to understand why this is OK where other, similar changes were not.

[line-o]

Has this been tested with the bundled applications?

[adamretter]

Changes like this were considered breaking in the past.
If in any application running on exist (a future version of 5) calls fn:matches((1,2), "\d") will now throw an error.

Good point!

Technically this is not an API change though (which would warrant a major version bump), as the API has stayed the same. It is however a behaviour change as you describe.

@line-o Would it be acceptable to you, if I removed the commit that introduces the FORX0003 error from this PR? The other commits should not be a problem for a hotfix version

[line-o]

fn:matches("a", "\d?") will also throw (FORX0003) but that is a different error than the one that is thrown when a sequence is used as the first argument (XPTY0004) no?

[reinhapa]

fn:matches("a", "\d?") will also throw (FORX0003) but that is a different error than the one that is thrown when a sequence is used as the first argument (XPTY0004) no?

@line-o is this blocking the merge?

[line-o]

I would welcome this change and make it a precedent for future changes. Allow patch releases that bring the xquery engine to spec compliance. Or we decide that this is a won't fix as it is changing existent behaviour that is fixed only in a major release.

[line-o]

Since the back port for v6 of this was already merged to develop-6.x.x we can already consider this ti be a precedent.
But we should make it clear in at least the release notes what did change and how that might affect existing queries and applications that rely on the current behavior.

[adamretter]

we can already consider this ti be a precedent.

I am not comfortable with that.

My approach here was to address the concerns of @line-o and remove the things that he felt were breaking changes. The other PR to 6.x.x got merged before I could address @line-o's request and so it was outside of my control.

[reinhapa]

we can already consider this ti be a precedent.

I am not comfortable with that.

My approach here was to address the concerns of @line-o and remove the things that he felt were breaking changes. The other PR to 6.x.x got merged before I could address @line-o's request and so it was outside of my control.

Seems I was to fast here, @adamretter if you want you could prepare a PR for a partial roll back for those parts you are not comfortable with or would this to complicated now?

[line-o]

@adamretter This PR is marked as ready-for-review and two approvals. If you think it should not be merged as is please, it would be good to communicate that clearly. Request changes, convert it to a draft PR or add the ON HOLD label.

[adamretter]

@dizzzz @reinhapa I believe this is now ready to merge please

[line-o]

@adamretter the last commit is 6 months old. Is it really this PR that you think is ready to be merged?

[line-o]

There are also failing tests. I would at least want to these those succeed before this PR is merged @dizzzz @reinhapa

[adamretter]

the last commit is 6 months old. Is it really this PR that you think is ready to be merged?

@line-o Yes, it is this PR.

There are also failing tests. I would at least want to these those succeed before this PR is merged @dizzzz @reinhapa

@line-o If you check, you will see that the failing tests are unrelated to this PR.

[line-o]

@adamretter could you explain what your intent is with this:

There are two changes that were considered breaking in the past:

• If in any application running on exist (a future version of 5) calls fn:matches((1,2), "\d") will now throw when a sequence is used as the first argument (XPTY0004).
• fn:matches("a", "\d?") will also throw (FORX0003)

You said you wanted to address those and I am now confused why this is ready to be merged with both of the above still in there.

[line-o]

@adamretter could you please rebase so that the tests can be run.