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.

Files: 
CommentFileSizeAuthor
#5 1313382-dsm-remove-5.patch2.68 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]
#3 1313382-dsm-change2debug-3.patch2.53 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]

Comments

Ashlar’s picture

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

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

Status:Needs work» Needs review
StatusFileSize
new2.53 KB
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]
tim.plunkett’s picture

Status:Needs review» Needs work

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

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]

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

can we get this commited?

pounard’s picture

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

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

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

tim.plunkett’s picture

Throw named exceptions.

pounard’s picture

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

merlinofchaos’s picture

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.