Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Ignore missing 'debug.file_link_formatter' service in Debug bundle#21292
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| <callmethod="setDisplayOptions"> | ||
| <argumenttype="collection"> | ||
| <argumentkey="fileLinkFormat"type="service"id="debug.file_link_formatter"></argument> | ||
| <argumentkey="fileLinkFormat"type="service"id="debug.file_link_formatter"on-invalid="ignore"></argument> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
nicolas-grekas commentedJan 22, 2017
core23 commentedJan 22, 2017
I'm a little bit busy right now. Hope I can have a look at this next weekend. |
f93ce7e tobdd59aeCompare| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctiongetXsdValidationBasePath() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctiongetNamespace() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed
| */ | ||
| publicfunctionprocess(ContainerBuilder$container) | ||
| { | ||
| if (!$container->hasDefinition('var_dumper.html_dumper')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could be!$container->hasDefinition('var_dumper.html_dumper') || !$container->hasDefinition('debug.file_link_formatter'), removing the next check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch
| $dumperDefinition->addMethodCall('setDisplayOptions',array( | ||
| array( | ||
| 'fileLinkFormat' =>$formatterDefintion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
| <argument>0</argument><!-- flags--> | ||
| <callmethod="setDisplayOptions"> | ||
| <argumenttype="collection"> | ||
| <argumentkey="fileLinkFormat"type="service"id="debug.file_link_formatter"></argument> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
much simpler would be to useon-invalid="ignore" instead of changing it in a compiler pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree with@stof. Looks like overkill to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was my first implementation#21292 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sorry about the bad suggestion!
stof commentedFeb 2, 2017
status: needs work |
core23 commentedFeb 3, 2017
Fixed :) |
chalasr commentedFeb 3, 2017
Looks good to me 👍 Status: needs review |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
xabbuh commentedFeb 6, 2017
👍 |
ef86dc8 to2d60309Comparexabbuh commentedFeb 6, 2017
Rereading the discussion at#21292 (comment) I think it would indeed be simpler to use the |
core23 commentedFeb 6, 2017
Fixed that |
nicolas-grekas commentedFeb 6, 2017
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/AThe 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