Ignore missing 'debug.file_link_formatter' service in Debug bundle #21292

Merged
merged 1 commit into from Feb 6, 2017

Conversation

Projects
None yet
7 participants
@core23
Contributor

core23 commented Jan 14, 2017

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

@core23 core23 changed the base branch from master to 3.2 Jan 14, 2017

@core23 core23 changed the title from Patch/debug ignore to Ignore missing 'debug.file_link_formatter' service in Debug bundle Jan 14, 2017

@@ -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>

This comment has been minimized.

@chalasr

chalasr Jan 14, 2017

Member

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

@chalasr

chalasr Jan 14, 2017

Member

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 nicolas-grekas added this to the 3.2 milestone Jan 16, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 22, 2017

Member

@core23 can you look at @chalasr's comment please?

Member

nicolas-grekas commented Jan 22, 2017

@core23 can you look at @chalasr's comment please?

@core23

This comment has been minimized.

Show comment
Hide comment
@core23

core23 Jan 22, 2017

Contributor

I'm a little bit busy right now. Hope I can have a look at this next weekend.

Contributor

core23 commented Jan 22, 2017

I'm a little bit busy right now. Hope I can have a look at this next weekend.

+ /**
+ * {@inheritdoc}
+ */
+ public function getXsdValidationBasePath()

This comment has been minimized.

@chalasr

chalasr Feb 1, 2017

Member

should be removed

@chalasr

chalasr Feb 1, 2017

Member

should be removed

+ /**
+ * {@inheritdoc}
+ */
+ public function getNamespace()

This comment has been minimized.

@chalasr

chalasr Feb 1, 2017

Member

should be removed

@chalasr

chalasr Feb 1, 2017

Member

should be removed

+ */
+ public function process(ContainerBuilder $container)
+ {
+ if (!$container->hasDefinition('var_dumper.html_dumper')) {

This comment has been minimized.

@chalasr

chalasr Feb 1, 2017

Member

could be !$container->hasDefinition('var_dumper.html_dumper') || !$container->hasDefinition('debug.file_link_formatter'), removing the next check

@chalasr

chalasr Feb 1, 2017

Member

could be !$container->hasDefinition('var_dumper.html_dumper') || !$container->hasDefinition('debug.file_link_formatter'), removing the next check

This comment has been minimized.

@core23

core23 Feb 3, 2017

Contributor

Good catch

@core23

core23 Feb 3, 2017

Contributor

Good catch

+
+ $dumperDefinition->addMethodCall('setDisplayOptions', array(
+ array(
+ 'fileLinkFormat' => $formatterDefintion,

This comment has been minimized.

@stof

stof Feb 2, 2017

Member

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)

@stof

stof Feb 2, 2017

Member

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>

This comment has been minimized.

@stof

stof Feb 2, 2017

Member

much simpler would be to use on-invalid="ignore" instead of changing it in a compiler pass

@stof

stof Feb 2, 2017

Member

much simpler would be to use on-invalid="ignore" instead of changing it in a compiler pass

This comment has been minimized.

@chalasr

chalasr Feb 2, 2017

Member

This comment has been minimized.

@fabpot

fabpot Feb 6, 2017

Member

I agree with @stof. Looks like overkill to me.

@fabpot

fabpot Feb 6, 2017

Member

I agree with @stof. Looks like overkill to me.

This comment has been minimized.

@core23

core23 Feb 6, 2017

Contributor

This was my first implementation #21292 (comment)

@core23

core23 Feb 6, 2017

Contributor

This was my first implementation #21292 (comment)

This comment has been minimized.

@stof

stof Feb 6, 2017

Member

@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).

@stof

stof Feb 6, 2017

Member

@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).

This comment has been minimized.

@chalasr

chalasr Feb 6, 2017

Member

sorry about the bad suggestion!

@chalasr

chalasr Feb 6, 2017

Member

sorry about the bad suggestion!

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 2, 2017

Member

status: needs work

Member

stof commented Feb 2, 2017

status: needs work

@core23

This comment has been minimized.

Show comment
Hide comment
@core23

core23 Feb 3, 2017

Contributor

Fixed :)

Contributor

core23 commented Feb 3, 2017

Fixed :)

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Feb 3, 2017

Member

Looks good to me 👍

Status: needs review

Member

chalasr commented Feb 3, 2017

Looks good to me 👍

Status: needs review

@nicolas-grekas

👍

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 6, 2017

Member

👍

Member

xabbuh commented Feb 6, 2017

👍

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 6, 2017

Member

Rereading the discussion at #21292 (comment) I think it would indeed be simpler to use the on-invalid attribute and drop the compiler pass.

Member

xabbuh commented Feb 6, 2017

Rereading the discussion at #21292 (comment) I think it would indeed be simpler to use the on-invalid attribute and drop the compiler pass.

@core23

This comment has been minimized.

Show comment
Hide comment
@core23

core23 Feb 6, 2017

Contributor

Fixed that

Contributor

core23 commented Feb 6, 2017

Fixed that

@stof

stof approved these changes Feb 6, 2017

@xabbuh

xabbuh approved these changes Feb 6, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 6, 2017

Member

Thank you @core23.

Member

nicolas-grekas commented Feb 6, 2017

Thank you @core23.

@nicolas-grekas nicolas-grekas merged commit 2201fbe into symfony:3.2 Feb 6, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
fabbot.io [braces] Is not configurable.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

nicolas-grekas added a commit that referenced this pull request Feb 6, 2017

bug #21292 Ignore missing 'debug.file_link_formatter' service in Debu…
…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

@core23 core23 deleted the core23:patch/debug-ignore branch Feb 6, 2017

@fabpot fabpot referenced this pull request Feb 17, 2017

Merged

Release v3.2.4 #21640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment