[Config] extracted the xml parsing from XmlUtils::loadFile into XmlUtils::parse #23485
Conversation
CONTRIBUTORS.md
Outdated
| @@ -1723,3 +1723,4 @@ Symfony is the result of the work of many people who made the code better | |||
| - Thomas BERTRAND (sevrahk) | |||
| - Matej Žilák (teo_sk) | |||
| - Vladislav Vlastovskiy (vlastv) | |||
| - Ole Rößner (basster) | |||
There was a problem hiding this comment.
As far as I know, this file is auto-generated, so doesn't need to be manually modified
|
|
||
| try { | ||
| return static::parse($content, $schemaOrCallable); | ||
| } catch (\InvalidArgumentException $ex) { |
There was a problem hiding this comment.
the convention is to name this var $e
| try { | ||
| return static::parse($content, $schemaOrCallable); | ||
| } catch (\InvalidArgumentException $ex) { | ||
| throw new \InvalidArgumentException( |
There was a problem hiding this comment.
I propose this instead:
if ('The XML is not valid.' !== $ex->getMessage()) {
throw $e;
}
throw new \InvalidArgumentException(sprintf('The XML file "%s" is not valid.', $file), 0, $e->getPrevious());|
@nicolas-grekas Is there any way, besides pushing changes, to retrigger the builds? I don't see how the failing test is related to my changes. |
| try { | ||
| return static::parse($content, $schemaOrCallable); | ||
| } catch (\InvalidArgumentException $e) { | ||
| if ('The XML is not valid.' !== $e->getMessage()) { |
There was a problem hiding this comment.
Logic sounds very fragile to me. Can we instead add a code to the exception and check the code here?
There was a problem hiding this comment.
sure, sounds comprehensible. @fabpot any preferences or guidelines for internal exception codes?
There was a problem hiding this comment.
No guidelines. I think we don't have any other examples in the framework. Another option would be to create a dedicated exception class.
There was a problem hiding this comment.
Sounds even cleaner to me, but wouldn't that result in a BC break?
There was a problem hiding this comment.
a dedicated exception class would have my preference
|
friendly ping |
|
@Basster can you rebase (and squash meanwhile if you want) please so that the merge commit disappears? |
3b3d1fd to
823eb20
Compare
From https://symfony.com/doc/current/contributing/code/patches.html, should I do so, anyways? |
|
Not if you don't want :)
But rebasing is needed as there is a conflict. |
|
@Basster would you have some time to rebase before the end of the week? |
823eb20 to
d8438a5
Compare
|
@nicolas-grekas done (hopefully correct) |
|
ping @symfony/deciders |
|
Thank you @Basster. |
…File into XmlUtils::parse (Basster) This PR was squashed before being merged into the 3.4 branch (closes #23485). Discussion ---------- [Config] extracted the xml parsing from XmlUtils::loadFile into XmlUtils::parse | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT I needed the xml parsing for an XML string generated in memory, so I extracted the actual parsing from XmlUtils::loadFile into XmlUtils::load. Re-opened after I messed some things up in #23482 Commits ------- 7473981 [Config] extracted the xml parsing from XmlUtils::loadFile into XmlUtils::parse
* 3.4: Passing the newly generated security token to the event during user switching. Fix changelog and minor tweak for #23485 [Config] extracted the xml parsing from XmlUtils::loadFile into XmlUtils::parse [Security][SecurityBundle] Deprecate the HTTP digest auth add ability to configure catching exceptions Extract method refactoring for ResourceCheckerConfigCache
I needed the xml parsing for an XML string generated in memory, so I extracted the actual parsing from XmlUtils::loadFile into XmlUtils::load.
Re-opened after I messed some things up in #23482