Some functions seems to be deprecated, therefore some dsm() have been put in order to warn if called: this code has been commited and if it runs on production, it might cause WSOD because dsm() won't exist without devel module: maybe triggering a watchdog notice or such could be better?

See ctools.module lines 681 and 689.

CommentFileSizeAuthor
#5 1313382-dsm-remove-5.patch2.68 KBandypost
#3 1313382-dsm-change2debug-3.patch2.53 KBandypost

Comments

Ashlar’s picture

Ashlar commented
Status: Active » Closed (won't fix)

This bug report has not been active for six months. In an effort to clean-up the issue queue this item has been closed. If your modules are current and the report is still relevant please feel free to change the Status back to active.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Closed (won't fix) » Needs work

Only a few places needs review

ctools# grep -r "dsm" ./
./ctools.module:  dsm('should not be called!');
./ctools.module:  dsm('should not be called!');
./page_manager/page_manager.module:      dsm($retval);
./page_manager/page_manager.admin.inc:        dsm($path);
./page_manager/page_manager.admin.inc:        dsm($item);
./page_manager/page_manager.admin.inc:        dsm($attributes['class']);
./includes/stylizer.inc://  dsm($processor->message_log);
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1313382-dsm-change2debug-3.patch2.53 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work

Why keep any of this? I'd say remove it completely.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1313382-dsm-remove-5.patch2.68 KB

Suppose this commented out code is a thing of a past.
Suppose we need to ping someone from ctools team to carefully review, probably this clean-up could be deeper.

andypost’s picture

andypost
he/him
Primary language Russian
commented

can we get this commited?

pounard’s picture

pounard
Primary language French
commented

Code that should not be called may throw logic error exception, to explicitely tell the code he did a bad mistake: finding errors right away forces the devs to fix them. I'm just sayin', I really don't know why you kept those functions in the first place, there's maybe a good reason.

merlinofchaos’s picture

merlinofchaos commented

It was a bad idea of me to use dsm() to point out these errors, but pounard is right -- it's better for developers to get explicit errors.

I don't remember why those functions are still there at this point. Possibly worth doing a little code archeaology to see.

andypost’s picture

andypost
he/him
Primary language Russian
commented

so... let's change them to trigger_error()?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Throw named exceptions.

pounard’s picture

pounard
Primary language French
commented

Depends on how is fatal the error, but if they are, exceptions are the way to go.

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Fixed

Okay, I went through the list. I removed the no longer needed context_cache functions. I replaced a dsm about $attributes['class'] with a correction to just make it an array. (Experience has shown that there's Always One More 'class' setting that was not updated to the array notation).

I left in the commented out dsm()s. In a way, they're a reminder of something I had intended but haven't gotten back to. Maybe someday I or someone will, and the commented code at least says something about it. Removing it guarantees nothing will ever happen there.

Committed and pushed.

Status: Fixed » Closed (fixed)

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