addHook saveReady question

[joe_g]

Hi there,

I have "events" with lots of children "times", to make some queries possible, I copy the earliest / latest time-child into the parent event with this code:

This works MOST of the time (99.9%), but the client has been complaining for a long time that this sometimes fails. I've been quite a lot with trying to reproduce the error with no luck. It's maybe because I don't know exactly how hooks work.

So, my question is, is this a safe way to do this? Or is it a bad idea to modify a page while saving, like this?

$time->startdate is mandatory, but the enddate isn't. Thats why I copy startdate to enddate, if enddate is empty.

thanks!

$pages->addHook('saveReady', function($event) {
	$p = $event->arguments(0);
	if($p->template->name == 'event') {
		$p->of(false);
		$enddate = 0;
		$startdate = 2147483646;
		foreach($p->times as $time) {
			if(!$time->enddate) {
				$time->enddate = $time->startdate;
			}
			if($time->enddate > $enddate) $enddate = $time->enddate;
			if($time->startdate < $startdate) $startdate = $time->startdate;
		}
		$p->enddate = $enddate;
		$p->startdate = $startdate;
     }
});

 

[joe_g]

tx I'll try that and see if it helps!

[kongondo]

1 hour ago, joe_g said:

tx I'll try that and see if it helps!

Oops. Sorry, I removed my post just as you were responding to it. I noticed you were hooking into saveReady so my response didn't make sense. Ignore me. You'll get better answers .

[Robin S]

9 hours ago, joe_g said:

This works MOST of the time (99.9%), but the client has been complaining for a long time that this sometimes fails. I've been quite a lot with trying to reproduce the error with no luck.

Fails in what way? What is the error that your client is seeing?

 

9 hours ago, joe_g said:

I have "events" with lots of children "times", to make some queries possible, I copy the earliest / latest time-child into the parent event

Could you clarify what you mean by child and parent? Child and parent pages? Not sure how that fits with the code you've shown. What type of field is "times"?

 

9 hours ago, joe_g said:

$pages->addHook('saveReady', function($event) {

I think this should be:

$pages->addHookAfter('saveReady', function($event) {

You only use "addHook" if you are adding a new method or property to the class.

 

You don't need to do $p->of(false) for the page being saved in a saveReady hook because output formatting is already false at this point. But it doesn't do any harm either and it isn't related to the problem you're having.

 

9 hours ago, joe_g said:

$time->startdate is mandatory, but the enddate isn't. Thats why I copy startdate to enddate, if enddate is empty.

Nothing is ever really mandatory when it comes to fields. Setting the "required" option for a field just means that the user will see an error when they save a page with this field empty. But the page is still saved with the required field in its empty state. So you should always account for the possibility that a required field may be empty. You could test what happens in your hook when you save a page with "startdate" empty. Perhaps that causes the issue?

[kongondo]

55 minutes ago, Robin S said:

I think this should be:

$pages->addHookAfter('saveReady', function($event) {

You only use "addHook" if you are adding a new method or property to the class.

This was my initial thought (in the now edited post) but then I read this from the Hooks docs:

Quote

Some hookable methods in ProcessWire have no implementation and exist purely for you to hook. Because hooks like this exist purely "to be hooked", it doesn't matter if you hook before or after them. Examples include Pages::saveReady which is called after it's been determined the page will definitely be saved, but right before it is done. Another is Pages::saved, which is called after a page has successfully been saved. These hooks provide more certainty and less need for error checking than hooking before or after Pages::save. You may see several other specific-purpose hooks like this in ProcessWire.

 

[Robin S]

35 minutes ago, kongondo said:

Because hooks like this exist purely "to be hooked", it doesn't matter if you hook before or after them.

You learn something new every day. 

[bernhard]

Thanks kongondo and robin, I also thought the hook was wrong and learned something from the docs

@adrian

maybe for the hook recorder it would be possible to see somehow if the hook is one that actually does anything (so before and after do matter) or that does nothing (like saveReady. Just thinking loud - no idea how complicated that would be. Don't think it would really be important - but if it was easy it could be a nice addition.

@joe_g 

don't know what could be the problem (dates are always confusing), but maybe the execution of the hook fails sometimes because you get a php error (like cannot call property of undefined etc...). maybe you can install tracy and make sure you define your email adress then tracy will send you all errors that occur on your site. maybe you can also find some helpful information in your site's error logs.

[joe_g]

Interesting!

On 1/9/2018 at 12:06 AM, Robin S said:

Could you clarify what you mean by child and parent? Child and parent pages? Not sure how that fits with the code you've shown. What type of field is "times"?

times is a repeater with "startdate" (date) "enddate" (date) and "occasion" (string). My function above loops through them to get the earliest start date and latest enddate, and store them in the parent page event,

On 1/9/2018 at 12:06 AM, Robin S said:

Fails in what way? What is the error that your client is seeing?

The clients adds a bunch of times (repeater), saves the "event", and then the times are gone. They would have to enter the times again.

Now after testing a bunch of times, it does seem to relate to required field: If i skip the required startdate, create 3 times, and press save one of the times was missing. But this only happened twice, then not at all when trying to 10 times more ! Strange, but not the end of the world. But it does seem that the required error message does affect saving a bit.....