Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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()
Comment | File | Size | Author |
---|
Issue fork drupal-2002514
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:
- 2002514-deprecate-debug-remove changes, plain diff MR !315
Comments
Comment #1
RobLoachComment #2
RobLoachComment #3
dawehnerIs it just me that this doesn't really belong in a string class?
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedi think this should be best fit for a debug component:)
Comment #5
dawehnerNice idea!
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedawesome, 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
Comment #8
dawehnerI'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.
Comment #9
damiankloip CreditAttribution: damiankloip commentedHow about we call it output? So we get
Debug::output(blah...)
? or maybeDebug::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.
Comment #10
damiankloip CreditAttribution: damiankloip commentedSorry, forgot to change the wrapper in common.inc
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedthis looks really awesome now, thanks!
if bot disagrees it ll let us know
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedreally minor doc nitpicks
Comment #13
damiankloip CreditAttribution: damiankloip commentedSweet
Comment #15
dawehnerSimpletest relies on _drupal_get_last_caller which has debug() hardcoded as blacklist.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
Comment #17
alexpottI 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.
Comment #18
dawehnerSo something like this?
Comment #19
chx CreditAttribution: chx commentedThere must be a limit to this madness, Debug::output, for reals? What the hell is wrong with
include_once core/includes/common.inc
?Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedthe madness here is the amount of php functions we wrap, not the fact that we move it to classes imo
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedtest looks good, and debug() function is not removed, so anyone can do #19 or, use namespaces/classes, w/e seems fit for him
Comment #22
chx CreditAttribution: chx commentedI 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.
Comment #23
chx CreditAttribution: chx commentedNope, I can't let this just pass. If you must do this then at least move
Debug::output
intoDebug::__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 writeDebug($variablename);
in any IDE that can autoimport namespaces and not lose anything compared to writingdebug($variablename);
Comment #24
alexpottI have to agree with chx here Debug::output() just does not smell right... let's use the magical
__invoke
and move on.Comment #25
dawehnerThe 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 :(
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedi 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)
Comment #28
chx CreditAttribution: chx commentedEh, 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.
Comment #29
alexpottWhy 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
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedre-removing tag
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
Comment #31
dawehnerI 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?
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedwhat 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
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedopened #2022931: Move drupal_var_export() to \Drupal\Component\Utility so this issue is not derailed anymore
Comment #34
dawehnerRemoved the drupal_var_export changes.
Comment #36
dawehner__invoke does not work with static objects ....
Comment #38
tim.plunkettComment #39
chx CreditAttribution: chx commentedComment #40
tim.plunkettI 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)
Comment #41
Mile23Out of scope aside: Here's the unit test for Variable: #2163245: Unit testing for \Drupal\Component\Utility\Variable
Comment #42
Mile23debug()
now just callsSafeMarkup::checkPlain()
withprint_r()
orvar_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 thedebug()
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 indebug()
, butvar_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. Butdebug()
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()
.Comment #46
joachim CreditAttribution: joachim commentedPatch based on #42.
Comment #48
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedDidn't change anything , just uploading this patch because last one failed.
Comment #54
Krzysztof DomańskiComment #56
andypostThis 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
Comment #59
mondrakeChanged to MR workflow and deprecated debug().
Comment #60
daffie CreditAttribution: daffie commentedBack to needs work for the unresolved threads.
Comment #61
mondrakeComment #62
daffie CreditAttribution: daffie commentedUpdated the CR with the deprecation of the global function debug().
All code changes look good to me.
Comment #63
alexpottSo we ended up with
dump()
:)Committed 3ac9df1 and pushed to 9.2.x. Thanks!