Ignore missing 'debug.file_link_formatter' service in Debug bundle #21292
Conversation
core23
changed the title
| @@ -38,7 +38,7 @@ | |||
| <argument>0</argument> <!-- flags --> | |||
| <call method="setDisplayOptions"> | |||
| <argument type="collection"> | |||
| <argument key="fileLinkFormat" type="service" id="debug.file_link_formatter"></argument> | |||
| <argument key="fileLinkFormat" type="service" id="debug.file_link_formatter" on-invalid="ignore"></argument> | |||
There was a problem hiding this comment.
If the service is invalid then the call still occurs with an empty collection as argument. I suggest to remove the call entirely and add it conditionally in DebugExtension instead
|
I'm a little bit busy right now. Hope I can have a look at this next weekend. |
core23
force-pushed
the
patch/debug-ignore
branch
3 times, most recently
from
f93ce7e to
bdd59ae
Compare
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getXsdValidationBasePath() |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getNamespace() |
| */ | ||
| public function process(ContainerBuilder $container) | ||
| { | ||
| if (!$container->hasDefinition('var_dumper.html_dumper')) { |
There was a problem hiding this comment.
could be !$container->hasDefinition('var_dumper.html_dumper') || !$container->hasDefinition('debug.file_link_formatter'), removing the next check
|
|
||
| $dumperDefinition->addMethodCall('setDisplayOptions', array( | ||
| array( | ||
| 'fileLinkFormat' => $formatterDefintion, |
There was a problem hiding this comment.
this is wrong. You must use a reference, not an inline definition (which would create another instance of the class, as it would be a different service)
| @@ -36,11 +36,6 @@ | |||
| <argument>null</argument> | |||
| <argument>%kernel.charset%</argument> | |||
| <argument>0</argument> <!-- flags --> | |||
| <call method="setDisplayOptions"> | |||
| <argument type="collection"> | |||
| <argument key="fileLinkFormat" type="service" id="debug.file_link_formatter"></argument> | |||
There was a problem hiding this comment.
much simpler would be to use on-invalid="ignore" instead of changing it in a compiler pass
There was a problem hiding this comment.
I agree with @stof. Looks like overkill to me.
There was a problem hiding this comment.
This was my first implementation #21292 (comment)
There was a problem hiding this comment.
@core23 I know. and a community review asked you to change it, but several core team members looking at your PR are preferring the first implementation until now (and other core team members have not said what they prefer).
|
status: needs work |
core23
force-pushed
the
patch/debug-ignore
branch
from
bdd59ae to
c99117b
Compare
|
Fixed :) |
|
Looks good to me 👍 Status: needs review |
|
👍 |
core23
force-pushed
the
patch/debug-ignore
branch
2 times, most recently
from
ef86dc8 to
2d60309
Compare
core23
force-pushed
the
patch/debug-ignore
branch
from
2d60309 to
2201fbe
Compare
|
Rereading the discussion at #21292 (comment) I think it would indeed be simpler to use the |
|
Fixed that |
|
Thank you @core23. |
…g bundle (core23) This PR was merged into the 3.2 branch. Discussion ---------- Ignore missing 'debug.file_link_formatter' service in Debug bundle | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A The new `HtmlDumper` requires the `debug.file_link_formatter` service which isn't available in older versions of the `DebugBundle` Commits ------- 2201fbe Ignore missing 'debug.file_link_formatter' service in Debug bundle
The new
HtmlDumperrequires thedebug.file_link_formatterservice which isn't available in older versions of theDebugBundle