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

ext/bcmath: Correctly modified an existing document #4163

Merged
merged 7 commits into from
Dec 1, 2024

Conversation

SakiTakamachi
Copy link
Member

Fixed and added exception behavior.
Also added a <literal> tag.

Comment on lines 55 to 64
<refsect1 role="errors">
&reftitle.errors;
<para>
This function throws a <exceptionname>ValueError</exceptionname> in the following cases:
<simplelist>
<member><parameter>num1</parameter> or <parameter>num2</parameter> is not a well-formed BCMath numeric string</member>
<member><parameter>scale</parameter> is outside the valid range</member>
</simplelist>
</para>
</refsect1>
Copy link
Member

Choose a reason for hiding this comment

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

Considering how often this is repeated I would use XIncludes to avoid the repetition

@SakiTakamachi
Copy link
Member Author

fixed

@Girgias
Copy link
Member

Girgias commented Nov 29, 2024

@SakiTakamachi I pushed a bunch of wording fixes and docs fixes which are existing issues, as I wanted to check locally that everything renders somewhat as expected.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Small nits, otherwise ok

Co-authored-by: Niels Dossche <[email protected]>
@Girgias Girgias merged commit ddb05f8 into php:master Dec 1, 2024
2 checks passed
@cmb69
Copy link
Member

cmb69 commented Dec 1, 2024

Hmm, the commit breaks configure for me (Windows, libxml2 2.10.3), due to "nested" xincludes. bcdivmod xincludes the error section from bcdiv which in turn xincludes the contents of the bcadd error section. The latter xinclude is not resolved here, so validation complains. I don't know whether that is a libxml2 issue, or Windows related, or whether I'm doing something wrong.

@leonardolara
Copy link
Contributor

leonardolara commented Dec 1, 2024

The same happens with me (Suse Linux, libxml 2.10.3, PHP 8.4.0). It is not processing nested xincludes. I processed locally repeating the $dom->xinclude(); (lines 780-787 in configure.php) and it builds successfully.
However it does not build in Github.

@cmb69
Copy link
Member

cmb69 commented Dec 1, 2024

Hmm, from https://discourse.gnome.org/t/libxml2-2-11-0-released/15123:

The XInclude engine has been reworked to properly support nested includes.

Confused.

@cmb69
Copy link
Member

cmb69 commented Dec 1, 2024

Indeed, works (for me) with libxml2 2.11.9; is our CI running libxml2 2.11, since the nested includes have been processed properly?

However it does not build in Github.

Well, apparently it does for this repo.

Still, I don't think we can require libxml2 2.11 (even php-src master still supports >= 2.9.4).

@leonardolara
Copy link
Contributor

leonardolara commented Dec 1, 2024

I meant I could not build the pt_BR repo in Github.
Locally, neither en nor 'pt_BR' build correctly.

I've just downgraded to libxml2 2.9.14 to match the repo config, but I get the same errors.

What we could do is a loop to redo xinclude() until it gets to zero includes in configure.

@alfsb
Copy link
Member

alfsb commented Dec 2, 2024

Just a heads up. I'm doing a big change on cofigure.php, that touchs XIncludes by xml:ids, and, indirectly, in duplicated IDs. It not handles recursive XInclude as now, but it seems necessary anyways. I will push a new version soon, that will delay (again) it's merging. And try to come up for a solution for recursive XIndludes in the following days.

The looping xinclude() solves it in all languages that normally builds? (I stopped testing doc-it and doc-ro, as they appear to be dead for more than six months).

@leonardolara
Copy link
Contributor

I tried a looping xinclude() that works for almost all languages that are built normally, except for ja, in which case it generates a XPointer issue that didn't exist before. So I gave up for now, it is a little beyond my understanding.

@Girgias
Copy link
Member

Girgias commented Dec 2, 2024

Still, I don't think we can require libxml2 2.11 (even php-src master still supports >= 2.9.4).

Why could we not? I don't see really an issue with the doc repo requiring a certain version of libxml, they fix (and break) stuff all the time so needing to support a whole range of versions is going to be impossible.

@SakiTakamachi
Copy link
Member Author

By the way, my local version is 2.9.4, but for some reason there are no problems...

@Girgias
Copy link
Member

Girgias commented Dec 2, 2024

Exactly what I said, the libxml CI and test coverage is quite bad, @nielsdos has reported multiple bugs upstream that broke the test coverage of DOM and related XML extensions when they did some fixes/refactorings.

@cmb69
Copy link
Member

cmb69 commented Dec 2, 2024

@Girgias, libxml2 2.11 is pretty new, so that might break builds for many users. Even Ubuntu 24.04 ships libxml2 2.9.14 by default. And you would have to ask @derickr (or someone else) to update the build machines. Might also be an issue for CHM building.

@Girgias
Copy link
Member

Girgias commented Dec 2, 2024

It seems the issue is just present in libxml 2.10, if Saki is able to build with 2.9, soooo.

@cmb69
Copy link
Member

cmb69 commented Dec 2, 2024

From #4163 (comment):

I've just downgraded to libxml2 2.9.14 to match the repo config, but I get the same errors.

Apparently, it sometimes works with libxml2 < 2.11, but sometimes it doesn't.

@alfsb
Copy link
Member

alfsb commented Dec 2, 2024

Apparently, it sometimes works with libxml2 < 2.11, but sometimes it doesn't.

I opened php/doc-base#194 , that patchs part the problem for translations (it's not related with recursive calls, but with failed XIncludes, that exacerbate the problem). It does not include looping over xinclude(), but local tests showed this is viable workaround for where libxml is failing to recurse.

@alfsb
Copy link
Member

alfsb commented Dec 2, 2024

Included on doc-base PR 194 above both fixes: tor recursive XIncludes (all languages) and failing XIncludes (all translations). Let me know it this fixes the two separate problems one go. @SakiTakamachi @cmb69 @leonardolara

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.

6 participants