Problem/Motivation

debug()'s docblock documentation refers to _drupal_debug_message(), which no longer exists.

Proposed resolution

- Remove the reference to _drupal_debug_message(), and also update the documentation as needed.
- deprecate debug() function, see #2795567-123: Use Symfony's VarDumper for easier test debugging with dump()

Remaining tasks

review and commit

User interface changes

no

API changes

deprecation of debug()

Issue fork drupal-2002514

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

FileSize
5.96 KB
RobLoach’s picture

Title: Make utility.inc Unit Testable » Make utility.inc's drupal_var_export() Unit Testable
dawehner’s picture

Issue tags: +PHPUnit

Is it just me that this doesn't really belong in a string class?

ParisLiakos’s picture

i think this should be best fit for a debug component:)

dawehner’s picture

FileSize
7.98 KB

Nice idea!

ParisLiakos’s picture

awesome, thanks!
how about instead of Debug::debug() it is Debug::triggerError() since it is essentially a trigger_error wrapper. this way no need to repeat the same word:P

Status: Needs review » Needs work

The last submitted patch, drupal-2002514-5.patch, failed testing.

dawehner’s picture

how about instead of Debug::debug() it is Debug::triggerError() since it is essentially a trigger_error wrapper. this way no need to repeat the same word:P

I'm not sure about that. If you want to trigger an error do it, but the idea why people use debug is that they want to just print out the value, so I don't know whether they relate trigger error with print out values.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
8.41 KB

How about we call it output? So we get Debug::output(blah...)? or maybe Debug::print()?

I have also moved the Debug unit tests into their own class. I think this makes sense, as StringTest is just testing the String utility.

damiankloip’s picture

FileSize
422 bytes
8.41 KB

Sorry, forgot to change the wrapper in common.inc

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

this looks really awesome now, thanks!
if bot disagrees it ll let us know

ParisLiakos’s picture

FileSize
604 bytes
8.44 KB

really minor doc nitpicks

damiankloip’s picture

Sweet

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2002514-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
10.29 KB

Simpletest relies on _drupal_get_last_caller which has debug() hardcoded as blacklist.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we need to test the changes made to _drupal_get_last_caller and since we want to PHPUnit all the things actual move this into the new Debug class.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.99 KB
12.64 KB

So something like this?

chx’s picture

There must be a limit to this madness, Debug::output, for reals? What the hell is wrong with include_once core/includes/common.inc?

ParisLiakos’s picture

the madness here is the amount of php functions we wrap, not the fact that we move it to classes imo

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

test looks good, and debug() function is not removed, so anyone can do #19 or, use namespaces/classes, w/e seems fit for him

chx’s picture

I won't reset this but if this is what phpunit requires then I am sure we made a good choice by not using it five years ago. This sort of code moving is pointless and makes things much harder to find. But I guess it's water under the bridge, it will get in as so many others did.

chx’s picture

Status: Reviewed & tested by the community » Needs work

Nope, I can't let this just pass. If you must do this then at least move Debug::output into Debug::__invoke. That's what it is for after all -- when you have no idea what to call the method, don't call it anything. This way we can just write Debug($variablename); in any IDE that can autoimport namespaces and not lose anything compared to writing debug($variablename);

alexpott’s picture

I have to agree with chx here Debug::output() just does not smell right... let's use the magical __invoke and move on.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.13 KB

The top one goal here is to make it as fast as possible to use this functionality (in terms of typing speed).

I just tried that out( removed the debug function to be sure) added the __invoke typed "Debug" and at least phpstorm never let me autoimport the class. One workaround is to have both Debug::__invoke and output().

Here is the storm bug reported found by chx: http://youtrack.jetbrains.com/issue/WI-9662

For some odd reasons this does not work :(

Status: Needs review » Needs work

The last submitted patch, drupal-2002514-24.patch, failed testing.

ParisLiakos’s picture

Issue tags: -PHPUnit Blocker

i dont even care, thats not a real phpunit blocker (just use var_dump() in your tests, debug doesnt makes sense...i never used drupal_var_export)

chx’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +PHPUnit Blocker

Eh, this is rapidly becoming a classic case of bikeshedding where small patches are torn apart while large ones get in. Let's commit #18 and move on.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Why don't we just remove the debug() change here... it was introduced in #4 and was scope creep anyways and actually totally unnecessary for phpunit and testability afaics.... and lets put it in a VariableExport class in Drupal\Component\Utility

ParisLiakos’s picture

Issue tags: -PHPUnit Blocker

re-removing tag

and lets put it in a VariableExport class in Drupal\Component\Utility

I believe Debug as class name is better, cause well, its more general and we can put more stuff there later
VariableExport kinda locks it there

dawehner’s picture

I kind of agree that drupal_var_export should be moved into VariableExport but there should be also a Debug class. Do you agree with that?

ParisLiakos’s picture

what about just removing it? it is not being used anywhere..and we should encourage people dropping drupalisms.drupal_var_export is one.
we already wasted too much energy on this
Debug, VarExport..just leave it there..we have more important stuff to unit test than an unused function

ParisLiakos’s picture

opened #2022931: Move drupal_var_export() to \Drupal\Component\Utility so this issue is not derailed anymore

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.91 KB

Removed the drupal_var_export changes.

Status: Needs review » Needs work

The last submitted patch, drupal-2002514-34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
8.12 KB

__invoke does not work with static objects ....

Status: Needs review » Needs work

The last submitted patch, drupal-2002514-36.patch, failed testing.

tim.plunkett’s picture

Title: Make utility.inc's drupal_var_export() Unit Testable » Make debug() and _drupal_get_last_caller() Unit Testable
chx’s picture

Issue tags: +sad chx
tim.plunkett’s picture

Issue tags: -sad chx

I reopened/rescoped #2022931: Move drupal_var_export() to \Drupal\Component\Utility to do what this issue was opened to do.
(Very weird scope shift here, everyone. No idea whose idea this was)

Mile23’s picture

Out of scope aside: Here's the unit test for Variable: #2163245: Unit testing for \Drupal\Component\Utility\Variable

Mile23’s picture

Title: Make debug() and _drupal_get_last_caller() Unit Testable » Update docblock for debug(); remove references to _drupal_debug_message().
Issue summary: View changes
Issue tags: -PHPUnit

debug() now just calls SafeMarkup::checkPlain() with print_r() or var_export() output. SafeMarkup::checkPlain() has a bunch of unit testing: http://cgit.drupalcode.org/drupal/tree/core/tests/Drupal/Tests/Component...

_drupal_debug_message() no longer exists, but the debug() docblock refers to it.

We do have Drupal\Component\Utility\Variable::export() which also has a lot of unit testing happiness, but is largely unused. It could be used in debug(), but var_export() is probably fine.

I'm not sure where that leaves the scope of this issue. :-) It's entirely possible to unit-test a global function, it just means you have to include the .inc file, in our case. But debug() is not at all complex, so it's pretty easy to just read. It's dependencies are either system functions or already unit-tested.

I'd say re-scope this to change the docblock and remove the reference to _drupal_get_last_caller().

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Patch based on #42.

Status: Needs review » Needs work
gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
485 bytes

Didn't change anything , just uploading this patch because last one failed.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Krzysztof Domański’s picture

Version: 8.9.x-dev » 9.1.x-dev
FileSize
474 bytes

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

andypost’s picture

Title: Update docblock for debug(); remove references to _drupal_debug_message(). » Deprecate debug(); remove references to _drupal_debug_message().
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Kill includes, +@deprecated, +Needs change record
Related issues: +#2795567: Use Symfony's VarDumper for easier test debugging with dump()

This needs rework to deprecation of the function according to just merged #2795567-123: Use Symfony's VarDumper for easier test debugging with dump()

Re-titled and updated summary

mondrake made their first commit to this issue’s fork.

mondrake’s picture

Issue summary: View changes
Status: Needs work » Needs review

Changed to MR workflow and deprecated debug().

daffie’s picture

Status: Needs review » Needs work

Back to needs work for the unresolved threads.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record +Needs change record updates
daffie’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

Updated the CR with the deprecation of the global function debug().

All code changes look good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

So we ended up with dump() :)

Committed 3ac9df1 and pushed to 9.2.x. Thanks!

  • alexpott committed 3ac9df1 on 9.2.x
    Issue #2002514 by dawehner, mondrake, damiankloip, RobLoach, ParisLiakos...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.