8000 Added check for ext-dom to XmlUtil::loadFile by oleg-andreyev · Pull Request #24047 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Added check for ext-dom to XmlUtil::loadFile #24047

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2017
Merged

Added check for ext-dom to XmlUtil::loadFile #24047

merged 1 commit into from
Sep 3, 2017

Conversation

oleg-andreyev
Copy link
Contributor
@oleg-andreyev oleg-andreyev commented Aug 31, 2017 •
edited by ogizanagi
Loading
Q A
Branch? 2.7
Bug fix? no, minor enhancement to gracefully suggest to install ext-dom if missing when trying to use XmlUtil::loadFile
New feature? no
BC breaks? no
Deprecations? no
Tests pass? ?
Fixed tickets #24046
License MIT

@oleg-andreyev oleg-andreyev force-pushed the add-dom-extension-to-composer branch from 3f45aee to f665f42 Compare August 31, 2017 09:35
oleg-andreyev
oleg-andreyev commented Aug 31, 2017
View reviewed changes
src/Symfony/Component/Config/composer.json Outdated
@@ -17,6 +17,7 @@
],
"require": {
"php": "^7.1.3",
"ext-dom": "*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

although DOM is installed together with php-xml, it's still possible to disable xml extension and enable dom extension

// test.php
<?php
use Symfony\Component\Config\Util\XmlUtils;
require_once 'vendor/autoload.php';
var_dump(XmlUtils::loadFile('test.xml'));

// test.xml
<?xml version="1.0" encoding="utf-8"?>
<root xmlns="http://example.com/schema"></root>

$ php -n -dextension=phar.so -dextension=json.so -dextension=iconv.so -dextension=xml.so test.php
Fatal error: Uncaught Error: Class 'DOMDocument' not found in /mnt/c/checkouts/symfony/src/Symfony/Component/Config/Util/XmlUtils.php:52
$ echo $? # => 255

$ php -n -dextension=phar.so -dextension=json.so -dextension=iconv.so -dextension=dom.so test.php
$ echo $? // => 0

@xabbuh
Copy link
Member
xabbuh commented Aug 31, 2017

It's perfectly valid to use the Config component without XML files. Thus I suggest to instead handle the missing XML extension case gracefully in the XmlUtils class instead.

@stof
Copy link
Member
stof commented Aug 31, 2017

FrameworkBundle should have the requirement though, because it depends on XML support.

@stof
Copy link
Member
stof commented Aug 31, 2017

the requirement should be ext-dom btw

@oleg-andreyev
Copy link
Contributor Author

@stof ext-xml requirements was added in #22676 and as @xabbuh mentioned it's valid use case to use symfony/config without ext-xml or ext-dom

@chalasr
Copy link
Member
chalasr commented Aug 31, 2017

What @stof means is that the requirement on FrameworkBundle should be ext-dom instead of ext-xml, that could be fixed here if you wish to do it.
For Config, we should just throw in XmlUtiils if !extension_loaded('xml') as suggested by @xabbuh .

@chalasr
Copy link
Member
chalasr commented Aug 31, 2017

Btw, this should be rebased on 2.7 and the target branch changed accordingly.

@chalasr chalasr added this to the 2.7 milestone Aug 31, 2017
@oleg-andreyev
Copy link
Contributor Author

@chalasr it's possible to disable xml and enable dom only, ref.: #24047 (comment), so changing FrameworkBundle requirements from ext-xml to ext-dom is quite dangerous

I can put !extension_loaded('dom') in the file, but I didn't done that initially because \Symfony\Component\Config\Util\XmlUtils::phpize is quite useful method :)

@stof
Copy link
Member
stof commented Aug 31, 2017

@oleg-andreyev the question is whether we need only ext-dom or also ext-xml. If we need ext-xml, the requirement should stay like that.

But anyway, for the config component, it must stay an optional dependency (the component contains many other things not requiring XML)

@chalasr
Copy link
Member
chalasr commented Aug 31, 2017 •
edited
Loading

For Config we should deal with xml only, not dom which is a requirement for FrameworkBundle only. So the check should be !extension_loaded('xml').

I didn't done that initially because \Symfony\Component\Config\Util\XmlUtils::phpize is quite useful method :)

Well, I don't have the code in mind, but if phpize() can be used without the xml extension, we should not forbid using it, adding the check only where it breaks already if xml extension not loaded. But I guess it does break, isn't it?

edit: let's let FrameworkBundle as is if dom doesn't guarantee xml, as said

@oleg-andreyev
Copy link
Contributor Author

@chalasr I don't see where ext-dom in requirements for FrameworkBundle.

sum up:

  • I've updated XmlUtils::loadFile because it's using \DOMDocument
  • I didn't update XmlUtils::phpize could be used without ext-xml or ext-dom
  • I didn't update XmlUtils::convertDomElementToArray, because it has input argument \DOMElement $element

8000
@oleg-andreyev oleg-andreyev force-pushed the add-dom-extension-to-composer branch from 6e18095 to 8bde9e2 Compare August 31, 2017 13:56
@oleg-andreyev oleg-andreyev changed the base branch from master to 2.7 August 31, 2017 13:56
chalasr
chalasr reviewed Aug 31, 2017
View reviewed changes
src/Symfony/Component/Config/Util/XmlUtils.php
*/
public static function loadFile($file, $schemaOrCallable = null)
{
if (!extension_loaded('dom')) {
Copy link
Member

Choose a reason for hiding this comment

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

you pointed out that dom doesn't guarantee xml. Should we put || !extension_loaded('xml') since this is calling xml functions?

Copy link
Contributor Author
@oleg-andreyev oleg-andreyev Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

yes, ext-dom does not guarantee that ext-xml is enabled, but php itself guarantees libxml, so all libxml_* will work, unless php is compiled from source with --disable-libxml (which is probably where uncommon)

$ apt-cache depends php7.0-fpm
php7.0-fpm
  Depends: libmagic1
  Depends: mime-support
  Depends: php7.0-cli
  Depends: php7.0-common
  Depends: php7.0-json
  Depends: php7.0-opcache
  Depends: tzdata
  Depends: ucf
  Depends: init-system-helpers
  Depends: lsb-base
  Depends: libapparmor1
  Depends: libc6
  Depends: libpcre3
  Depends: libssl1.0.0
  Depends: libsystemd0
  Depends: libxml2
  Depends: zlib1g
  Conflicts: <php5-fpm>
  Sugges
8000
ts: php-pear

$ apt-cache depends php7.0-cli
php7.0-cli
  Depends: libedit2
  Depends: libmagic1
  Depends: mime-support
  Depends: php7.0-common
  Depends: php7.0-json
  Depends: php7.0-opcache
  Depends: php7.0-readline
  Depends: tzdata
  Depends: ucf
  Depends: libc6
  Depends: libpcre3
  Depends: libssl1.0.0
  Depends: libxml2
  Depends: zlib1g
  Breaks: <php5-cli>
  Suggests: php-pear
  Replaces: <php5-cli>

$ apt-cache depends php7.0-cgi
php7.0-cgi
  Depends: libmagic1
  Depends: mime-support
  Depends: php7.0-cli
  Depends: php7.0-common
  Depends: php7.0-json
  Depends: php7.0-opcache
  Depends: tzdata
  Depends: ucf
  Depends: libc6
  Depends: libpcre3
  Depends: libssl1.0.0
  Depends: libxml2
  Depends: zlib1g
  Conflicts: <php5-cgi>
  Suggests: php-pear

Copy link
Member

Choose a reason for hiding this comment

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

ok then :)

@chalasr
Copy link
Member
chalasr commented Aug 31, 2017 •
edited
Loading

The existing tests for this method should be updated to add @requires extension dom (and/or xml) so that the component test suite can be ran even if the extension is not loaded.

@oleg-andreyev
Copy link
Contributor Author

add @requires extension dom

@chalasr I'm a bit surprised, but without ext-dom we won't be able to run phpunit
Ref.: \PHPUnit\Util\Xml::load, lol

@chalasr
Copy link
Member
chalasr commented Sep 1, 2017

@oleg-andreyev fair enough 😅

@chalasr chalasr requested a review from ogizanagi September 1, 2017 21:40
@chalasr
Copy link
Member
chalasr commented Sep 2, 2017

@oleg-andreyev Can you rebase your branch against 2.7 to see tests green?

@ogizanagi ogizanagi force-pushed the add-dom-extension-to-composer branch from 8bde9e2 to 2f292c2 Compare September 3, 2017 10:03
@ogizanagi ogizanagi changed the title adding ext-xml to symfony/config Added check for ext-dom to XmlUtil::loadFile Sep 3, 2017
@ogizanagi
Copy link
Contributor

Thank you @oleg-andreyev.

@ogizanagi ogizanagi merged commit 2f292c2 into symfony:2.7 Sep 3, 2017
ogizanagi added a commit that referenced this pull request Sep 3, 2017
@ogizanagi
minor #24047 Added check for ext-dom to XmlUtil::loadFile (oleg-andre…
26ef6d2 
…yev)

This PR was merged into the 2.7 branch.

Discussion
----------

Added check for ext-dom to XmlUtil::loadFile

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no, minor enhancement to gracefully suggest to install `ext-dom` if missing when trying to use `XmlUtil::loadFile`
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | ?
| Fixed tickets | #24046
| License       | MIT

Commits
-------

2f292c2 #24046 added check for ext-dom to XmlUtil::loadFile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogizanagi ogizanagi ogizanagi approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Config Status: Reviewed
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
6 participants
@oleg-andreyev @xabbuh @stof @chalasr @ogizanagi @carsonbot
0