Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DependencyInjection][Translator] Silent deprecation triggered by libxml_disable_entity_loader#39068

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.4fromjderusse:fix-xml
Nov 27, 2020

Conversation

@jderusse
Copy link
Member

@jderussejderusse commentedNov 12, 2020
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#39040
LicenseMIT
Doc PR-

The XML entity loader is disabled by default since libxml 2.9
But, since PHP 8.0, calling the methodlibxml_disable_entity_loader triggers a deprecation which has been solved in symfony by callinglibxml_disable_entity_loader only for libxml < 2.9

The issue is, some dependencies, enable the entity loader and does not restore the initial state afterward, leading to exceptions triggered by Symfony iteself.

In previous versions symfony was resilient by disabling the flag before working, which is not the case anymore to avoid the deprecation.

This PR restore the resiliency of Symfony for PHP < 8.0, which is not yet deprecated.

But we have no way to check the status of the entity loader without triggering a deprecation with Symfony 8.

@stof
Copy link
Member

could we suffer from issues on PHP 8 due to other libraries changing such global state too ? Or is PHP 8 turning that into a no-op ?

@jderusse
Copy link
MemberAuthor

@stof I don't think so, we will suffer from the same issue.

a solution could be to implement a method by our self that check the status of the entity loader and use this instead of checking the version.
note:libxml_disable_entity_loader is used in 5 components

@derrabus
Copy link
Member

could we suffer from issues on PHP 8 due to other libraries changing such global state too ?

Yes, this change only buys us some time. The main problem is that php 8 triggers a seprecation without offering us an alternative.

Or is PHP 8 turning that into a no-op ?

As far as I understood it, the functionality is the same.

@jderusse The PR title tells quite the opposite of what this PR is doing. 😉

@stof
Copy link
Member

Yes, this change only buys us some time. The main problem is that php 8 triggers a seprecation without offering us an alternative.

Then this should be reported to PHPbefore the release of PHP 8. Deprecating an API without an alternative is not OK.

derrabus reacted with thumbs up emoji

@jderusse
Copy link
MemberAuthor

Yes, this change only buys us some time. The main problem is that php 8 triggers a seprecation without offering us an alternative.

Then this should be reported to PHPbefore the release of PHP 8. Deprecating an API without an alternative is not OK.

This is deprecated, because nobody should call it in PHP >= 8.
BUT if somebody call it, you have to revert the call, and you can't do it without triggering a deprecation.

@stof
Copy link
Member

This is deprecated, because nobody should call it in PHP >= 8.
BUT if somebody call it, you have to revert the call, and you can't do it without triggering a deprecation.

That's precisely the issue. As it is deprecated without being a no-op, it still affects the global state for everyone. And so shared libraries (which cannot know what else is running in the project) cannot assume the global state and so are forced to keep calling the deprecated API. And that makes the deprecation warning an issue.
In practice, Symfony on PHP 8 is still not reliably working even after this PR, because it assumes that no other code is using the deprecated API.

@jderusse
Copy link
MemberAuthor

Bug openedhttps://bugs.php.net/bug.php?id=80357

derrabus reacted with thumbs up emoji

Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

I'm okay with the change because it fixes the regression for now. We can iterate further when we have feedback from PHP.

@derrabus
Copy link
Member

Bug openedhttps://bugs.php.net/bug.php?id=80357

Thank you. 😃

@jderussejderusse changed the titleCall libxml_disable_entity_loader only for PHP 8Silent deprecation triggered by libxml_disable_entity_loaderNov 12, 2020
@jderussejderusseforce-pushed thefix-xml branch 2 times, most recently from0fd32cc to137e31fCompareNovember 12, 2020 21:01
@jderusse
Copy link
MemberAuthor

jderusse commentedNov 12, 2020
edited
Loading

I changed the patch to

  1. let previous code aboutlibxml_disable_entity_loader(true)

Disabling entity loaded is not needed sin libXml 2.9 (this is the reason on the deprecation in PHP 8 by the way).
We don't have to disable it on our side (whether or not it's previously disabled, it won't changed our code).

  1. Silent deprecation triggered bylibxml_disable_entity_loader(false)

FALSE is default value, and Symfony need it (cf#39040). Given we can't do it without triggering a deprecation we shoud silent it, as suggested inhttps://bugs.php.net/bug.php?id=80357

  1. Stop callinglibxml_disable_entity_loader(false) after PHP 9

Because the method is deprecated in 8.0 and we are still calling it with@ sign, we should prevent calling it afterward.

@derrabus
Copy link
Member

So, it's silencing now according to the feedback we've received? Well okay, let's silence the error then.

@carsonbotcarsonbot changed the titleSilent deprecation triggered by libxml_disable_entity_loader[DependencyInjection][Translator] Silent deprecation triggered by libxml_disable_entity_loaderNov 12, 2020
@carsonbotcarsonbot changed the titleSilent deprecation triggered by libxml_disable_entity_loader[DependencyInjection][Translator] Silent deprecation triggered by libxml_disable_entity_loaderNov 12, 2020
@jderusse
Copy link
MemberAuthor

I don't know if PHP 8 will fix the issue, I the meantime, I suggest this patch that check if the entity loader is disabled by trying to validate a wellknown document

@fabpot
Copy link
Member

PHP won't fix the issue as PHP 8 is now released. Let's merge this one then.

@fabpot
Copy link
Member

Thank you@jderusse.

@fabpotfabpot merged commit6db84b6 intosymfony:4.4Nov 27, 2020
This was referencedNov 29, 2020
@jderussejderusse deleted the fix-xml branchDecember 9, 2020 22:10
ruudk added a commit to SimpleBus/SimpleBus that referenced this pull requestDec 10, 2020
ruudk added a commit to SimpleBus/SimpleBus that referenced this pull requestDec 10, 2020
ruudk added a commit to SimpleBus/SimpleBus that referenced this pull requestDec 10, 2020
ruudk added a commit to SimpleBus/SimpleBus that referenced this pull requestDec 10, 2020
@JarJak
Copy link

JarJak commentedDec 11, 2020
edited
Loading

@jderusse
Copy link
MemberAuthor

These 2 lines are about disabling theentity_loader which is not needed, because disabled by default since LIBXML_VERSION 2.9

This PR is about enabling theentity_loader in case somebody disabled it.

cmodijk pushed a commit to SimpleBus/SimpleBus that referenced this pull requestDec 13, 2020
cmodijk pushed a commit to SimpleBus/SimpleBus that referenced this pull requestDec 13, 2020
cmodijk pushed a commit to SimpleBus/asynchronous-bundle that referenced this pull requestDec 15, 2020
cmodijk pushed a commit to SimpleBus/jms-serializer-bundle-bridge that referenced this pull requestDec 15, 2020
cmodijk pushed a commit to SimpleBus/symfony-bridge that referenced this pull requestDec 15, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@derrabusderrabusderrabus approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@wouterjwouterjAwaiting requested review from wouterjwouterj is a code owner

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

7 participants

@jderusse@stof@derrabus@fabpot@JarJak@Nyholm@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp