timer_read() returns NULL (no value) after timer_stop() [#84008]

Problem/Motivation

timer_read() currently does not work after timer_stop().

Proposed resolution

Use $timers[$name]['time'] inside time_read() and timer_stop() instead of solely relying on $timers[$name]['start'];

Remaining tasks

Backport to d6

User interface changes

none

API changes

none

Original report by @plumbley

timer_read() currently does not work after timer_stop().

This has potential for confusion if by design, so perhaps the documentation should warn against it. E.g. instead of "Read the current timer value without stopping the timer." it could add "Important: the timer MUST be running when timer_read. is called".

To replicate, try e.g.

timer_start('test1'); 
echo timer_read('test1') . '<br />';
timer_stop('test1'); 
echo timer_read('test1');

which seems a reasonable thing to do, but you get e.g.
------
0.04
Notice: Undefined index: start in c:\program files\apache group\apache\htdocs\drupal\includes\bootstrap.inc on line 58
1158186408044.1
------
It looks like this is also true for CVS as well as 4.7.3. Mark.

Comment	File	Size	Author
#28	timer-stop-84008-28.patch	812 bytes	jacob.embree
#28
#23	timer_stop_D6.patch	1002 bytes	mikeryan
#23
#21	84008_accumulate_timer_value_5.x_6.x.patch	834 bytes	deviantintegral
#21
#18	84008_accumulate_timer_value.patch	2.6 KB	deviantintegral
#18
#16	84008_accumulate_timer_value.patch	2.61 KB	deviantintegral
#16
#14	84008_accumulate_timer_value.patch	2.61 KB	deviantintegral
#14
#11	84008_accumulate_timer_value.patch	2.46 KB	deviantintegral
#11
#9	issue-1063313.patch	698 bytes	lambic
#9
#6	issue-1063313.patch	749 bytes	lilou
#6
#1	bootstrap.inc_17.patch	689 bytes	plumbley
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

plumbley CreditAttribution: plumbley commented 14 September 2006 at 10:21

Version:	4.7.3	» x.y.z
Status:	Active	» Needs review

File	Size
bootstrap.inc_17.patch	689 bytes

Actually this is due to $timers[$name]['start'] being unset by timer_stop(), breaking the logic of the following line lin timer_read():

  $diff = round(($stop - $timers[$name]['start']) * 1000, 2);

So its actually easy to change this to make it work whether the timer is running or not - simply set $diff here to 0 if $timers[$name]['start'] is not set, i.e.

  $diff = isset($timers[$name]['start']) ? round(($stop - $timers[$name]['start']) * 1000, 2) : 0;

Patch attached fixes this for CVS of 14 Sept 2006 (Similar applies to 4.7.3, but I realized it may be a bit pointless for 4.7.3 now with something so minor). Mark.

Comment #2

drumm

he/him

NY, US

CreditAttribution: drumm commented 26 December 2006 at 00:17

Version:	x.y.z	» 6.x-dev
Status:	Needs review	» Needs work

patching file includes/bootstrap.inc
Hunk #1 FAILED at 59.
1 out of 1 hunk FAILED -- saving rejects to file includes/bootstrap.inc.rej

Comment #3

plumbley CreditAttribution: plumbley commented 29 December 2006 at 16:43

Title:	timer_read() returns garbage after timer_stop()	» timer_read() returns NULL (no value) after timer_stop()
Category:	bug	» feature
Status:	Needs work	» Active

Patch no longer applies, since a version was fixed by chx a couple of months later (version 1.129, Sun Nov 12 00:21:15 2006 UTC).

However, the new implementation of timer_read() does not return a value (i.e. implicitly returns NULL) after timer_stop() instead. This seems strange logic to me - why not allow the timers to be read after timer_stop()?

I have marked it as a "feature request" now instead with updated title (since it would be added functionality), but please mark as closed if this is the intended behaviour.

Best wishes, Mark.

Comment #4

Pasqualle

🇭🇺 Budapest

CreditAttribution: Pasqualle commented 16 September 2008 at 03:25

Version:	6.x-dev	» 7.x-dev
Priority:	Minor	» Normal

Comment #5

lilou CreditAttribution: lilou commented 17 October 2008 at 00:11

Status:

Active

» Fixed

Comment #6

lilou CreditAttribution: lilou commented 17 October 2008 at 00:12

Category:	feature	» bug
Status:	Fixed	» Needs review

File	Size
issue-1063313.patch	749 bytes

oups ...

Comment #7

16 November 2008 at 21:50

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #8

EvanDonovan CreditAttribution: EvanDonovan commented 28 July 2009 at 21:10

This is a very confusing situation (that timer_read() doesn't work if timer_stop() has been called). Could someone please re-roll the patch?

Also, could the documentation be updated for 6.x, or, even better, could the fix be backported after it's been applied to 7.x?

Comment #9

lambic CreditAttribution: lambic commented 21 September 2009 at 14:58

Status:

Needs work

» Needs review

File	Size
issue-1063313.patch	698 bytes

patch looks ok, re-submitting for the bot

Comment #10

deviantintegral CreditAttribution: deviantintegral commented 17 October 2009 at 00:01

Status:

Needs review

» Needs work

As-is, this patch appears to be a no-op; I'm still getting NULL after timer_stop().

Comment #11

deviantintegral CreditAttribution: deviantintegral commented 17 October 2009 at 00:39

Status:

Needs work

» Needs review

File	Size
84008_accumulate_timer_value.patch	2.46 KB

The documentation makes it seem as if the total running time for a single timer will cause the total time to be accumulated. DX frustration ensues :). The attached patch changes the timer_* functions as well as adds tests for them. The tests are still "needs work", but I'd like feedback as to if this is appropriate for D7 or if it should go to D8.

Comment #12

moshe weitzman CreditAttribution: moshe weitzman commented 17 October 2009 at 01:27

As long as this is a bug report, it can be committed to D7 before the release or afterwars. There is no freeze for bugs.

Comment #13

Dries CreditAttribution: Dries commented 17 October 2009 at 02:32

Yep, I'd consider this a bug.

Comment #14

deviantintegral CreditAttribution: deviantintegral commented 17 October 2009 at 14:08

File	Size
84008_accumulate_timer_value.patch	2.61 KB

Great. Here is an updated patch which includes function comments, and changes the test to be >= instead of >.

I've chosen 3 seconds for the sleep(), but that was just me choosing a number. Could we reduce it to 1?

Comment #15

17 October 2009 at 14:15

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #16

deviantintegral CreditAttribution: deviantintegral commented 17 October 2009 at 14:34

Status:

Needs work

» Needs review

File	Size
84008_accumulate_timer_value.patch	2.61 KB

Not sure why the above didn't apply, but I've updated from CVS and it passes tests fine locally.

Comment #17

Dries CreditAttribution: Dries commented 17 October 2009 at 17:34

We should probably use a smaller value, yes. I'd think 1 second ought to be enough, if not, use 2 seconds.

Comment #18

deviantintegral CreditAttribution: deviantintegral commented 17 October 2009 at 18:47

File	Size
84008_accumulate_timer_value.patch	2.6 KB

Here's an update with sleep(1) instead.

Comment #19

Dries CreditAttribution: Dries commented 18 October 2009 at 11:36

Looks good to me. Committed to CVS HEAD. Thanks!

Comment #20

Dries CreditAttribution: Dries commented 18 October 2009 at 11:40

Status:

Needs review

» Fixed

Comment #21

deviantintegral CreditAttribution: deviantintegral commented 18 October 2009 at 18:46

Version:	7.x-dev	» 6.x-dev
Status:	Fixed	» Needs review

File	Size
84008_accumulate_timer_value_5.x_6.x.patch	834 bytes

I think this should be backported to D6 and D5 as a bug fix. The attached patch is identical to Drupal 7, minus the simpletest. It applies cleanly to Drupal 5 and Drupal 6.

Comment #22

moshe weitzman CreditAttribution: moshe weitzman commented 27 October 2009 at 22:04

Status:

Needs review

» Reviewed & tested by the community

Comment #23

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan commented 23 February 2010 at 19:23

Status:

Reviewed & tested by the community

» Needs review

File	Size
timer_stop_D6.patch	1002 bytes

Updated patch fixing #723436: Timers accumulate at double-speed.

Comment #24

kenorb CreditAttribution: kenorb commented 14 October 2010 at 14:36

similar issue.
subscribe.

My case:

function hook_custom() {
  timer_start('visitorpath');
  // some code
  var_dump(timer_stop('visitorpath'));
}

function hook_exit() {
   var_dump('stop', timer_stop('visitorpath')); exit; // or timer_read - whatever
}

It returns:

array
  'count' => int 1
  'time' => float 82.75
array
  'count' => int 2
  'time' => float 160.29
array
  'count' => int 2
  'time' => float NULL

Workaround is to always start timer before you stop it:

timer_start('visitorpath'); // ad this line
$timer = timer_stop('visitorpath');
var_dump('stop', $timer); exit;

Comment #25

3 November 2010 at 12:11

Status:

Needs review

» Needs work

The last submitted patch, 84008_accumulate_timer_value_5.x_6.x.patch, failed testing.

Comment #26

EvanDonovan CreditAttribution: EvanDonovan commented 3 November 2010 at 12:51

Status:

Needs work

» Needs review

Patches for versions of Drupal prior to D7 will always fail testing.

Comment #27

thedavidmeister CreditAttribution: thedavidmeister commented 8 May 2014 at 13:24

Issue summary:	View changes
Status:	Needs review	» Needs work
Issue tags:		+Needs reroll, +Novice

patch does not apply anymore:

tdm:d6 thedavidmeister$ git apply timer_stop_D6.patch 
error: bootstrap.inc: No such file or directory

but this should be RTBC with a straight re-roll.

Comment #28

jacob.embree CreditAttribution: jacob.embree commented 17 February 2015 at 17:00

Status:	Needs work	» Needs review
Issue tags:	-Needs reroll

File	Size
timer-stop-84008-28.patch	812 bytes

4 files were hidden/shown/deleted

File	Size
84008_accumulate_timer_value.patch	2.61 KB

84008_accumulate_timer_value.patch	2.61 KB

84008_accumulate_timer_value.patch	2.6 KB

84008_accumulate_timer_value_5.x_6.x.patch	834 bytes

Reroll

Comment #29

thedavidmeister CreditAttribution: thedavidmeister commented 7 March 2015 at 08:33

Status:

Needs review

» Reviewed & tested by the community

Comment #30

7 March 2015 at 08:35

The last submitted patch, 23: timer_stop_D6.patch, failed testing.

Comment #31

2 March 2016 at 22:18

Status:

Reviewed & tested by the community

» Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.