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: Avoid nesting includes #4214

Closed

Conversation

SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi commented Dec 2, 2024

And added the title for error section (that forgot)

Related #4163 (comment)

cc: @cmb69 @leonardolara

@SakiTakamachi SakiTakamachi force-pushed the bcmath/avoid_nesting_includes branch from 710fa08 to 5b9eda3 Compare December 2, 2024 05:18
@cmb69
Copy link
Member

cmb69 commented Dec 2, 2024

Thank you @SakiTakamachi! I prefer this over calling DOMDocument::xinclude() multiple times to work around issues with older libxml. I still wonder why CI passed without this patch.

@alfsb
Copy link
Member

alfsb commented Dec 2, 2024

We can please hold off merging this for today? I think there two issues at play here, and will need two separated general fix.

  1. The current installed libxml2 we use does not expect recursive XIncludes
  2. Missing or empty <xi:fallback/> leaves <xi:include/> or empty holes behind, and this fails valuation.

This explains why doc-en pass with libxml2 upgrade or looping xinclude(), and translations not: there is no XInclude fails on doc-en.

I think I can send a PR for doc-base fixing both issues at general level.

@SakiTakamachi
Copy link
Member Author

@alfsb
Got it. It's already late in Japan, so there's no problem :)

@Girgias
Copy link
Member

Girgias commented Dec 2, 2024

Same I would rather not, libxml being buggy should not be the reason that we duplicate identical stuff and thus can have it out of sync.

@nielsdos
Copy link
Member

nielsdos commented Dec 2, 2024

I agree with @Girgias . Although imo the real sin is distributions sticking with ancient versions, which is a pain for both users and me.

@alfsb
Copy link
Member

alfsb commented Dec 2, 2024

I think I can send a PR for doc-base fixing both issues at general level.

Opened PR 194, linked above, that may supersedes this. Pass all languages on CI (that are building before), and seems to work well in Windows and Linux. @SakiTakamachi , please test it, to see if it fixes the manual building, without reverting the changes.

@SakiTakamachi
Copy link
Member Author

@alfsb
I was able to test your patch and confirm that it works perfectly
Thanks!

@SakiTakamachi
Copy link
Member Author

So, I close this PR :)

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.

5 participants