Putting page hooks in Page Classes

[MarkE]

I'm sure I read something about this recently, but I can't find it ... (is there a way of bookmarking forum posts?)

I'm trying to move my page hooks out of ready.php into Page Classes, which I think are tremendous and use extensively. But what is the best way to do it? ready() doesn't trigger automatically. I tried 

    public function ___loaded() {
        $this->ready();
    }

(thanks to @Robin S) and this does trigger it. However, if I do, say 

    public function ready() {
        $this->addHookAfter('Pages::save', $this, 'afterSave');
        $this->addHookBefore('Pages::save', $this, 'beforeSave');
    }

then the hook methods get triggered by any page save, not just of instances of this class. I solved this by adding a breakout at the start of the method:

    protected function beforeSave(HookEvent $event) {
        $p = $event->arguments(0);
        if ($this != $p) return;
......

but this seems a bit inelegant. Any better ideas?

[szabesz]

4 hours ago, MarkE said:

public function ___loaded() {

I have just posted about my findings regarding ___loaded(), see: https://processwire.com/talk/topic/25343-putting-page-hooks-in-page-classes/ Note that I used the latest dev (3.0.174) to do my testings.

4 hours ago, MarkE said:

then the hook methods get triggered by any page save, not just of instances of this class.

The issue is that same I described in my post, I think. ___loaded() gets called for any pages when using "direct subclasses" whereas going further down the class hierarchy, this is no longer the case.

Regarding hooks, I found that applying them in the constructor works as expected, at least the following way:

//direct subclass
class DefaultPage extends Page {}

plus:

//subclass of direct subclass
class ArticlesPage extends DefaultPage {

	public function __construct(Template $tpl = null) {
		parent::__construct($tpl);
		$this->wire->addHookAfter("Pages::saveReady", $this, "onSaveReady");
	}

	//this one gets called as expexted upon saving the "Articles" page but not when saving others
	public function onSaveReady(HookEvent $event) {
		$page = $event->arguments(0);
		bd($page); //Tracy Debugger is required for this one :)
	}
}

 

Edited March 26, 2021 by szabesz
typo fix

[MarkE]

    Snap!!

33 minutes ago, szabesz said:

think. ___loaded() gets called for any pages when using "direct subclasses" whereas going further down the class hierarchy, this is no longer the case.

I find it is the case for both direct and subclasses

33 minutes ago, szabesz said:

Regarding hooks, I found that applying them in the constructor works

Nice plan - quite neat and maybe better than using ___loaded() - but I still find the called method operates on all pages, so I need to filter with

    protected function beforeSave(HookEvent $event) {
        $p = $event->arguments(0);
        if ($this != $p) return;

It may be that my use case is rather different and that, in other circumstances, your solution works. In my case a button on the page executes a method which saves pages of a different class, but these still get caught by the hook.

EDIT: I find that @bernhard's solution has the same issue in my use case. The problem does not 'appear' to exist when using ready.php, because you naturally do $p = $event->arguments(0); there anyway and test for template before passing to the class method.

[szabesz]

@MarkE Have you perhaps done anything "not so documented" besides simply placing your class files in /site/classes/?

For example, in an effort to relocate the class files, I tried to do this:

$classLoader->addSuffix('Page', __DIR__ . "/templates/abode/{$template}");

as suggested by @teppo over here but doing so somehow results in the behavior you describe, that is: there is no difference between direct subclasses and subclasses of those in this regard we are discussing right now.

Can you please provide more details about what and how you are trying to achieve so that I can also test that?

 

[MarkE]

3 hours ago, szabesz said:

simply placing your class files in /site/classes/

That's where they are. I am also trying 

$this->addHookAfter('Pages::save(template=TemplateName)', $this, 'afterSave');

but can't get it to work at the moment.

[bernhard]

Please see my post here

 

9 hours ago, MarkE said:

Nice plan - quite neat and maybe better than using ___loaded() - but I still find the called method operates on all pages, so I need to filter with

    protected function beforeSave(HookEvent $event) {
        $p = $event->arguments(0);
        if ($this != $p) return;

That's correct behaviour. You are attaching the hook in your custom page class, but the hook you are attaching belongs to "Pages" and not to the custom page class.

If you attach a Pages::saveReady hook, it will always fire on Pages::saveReady having the saved page as arguments(0) and therefore you need to check if that page is your custom pageclass or not.

BTW I'd recommend doing this instead of $this != $p

$page = $event->arguments(0);
if(!$page instanceof self) return;

I've had problems using $page != $this when some properties of my page have changed and therefore the check returned false even though it was an instance of my custom pageclass. "instanceof" should always work as expected. You could also just check for $page->template if you prefer...

 

[bernhard]

12 minutes ago, MarkE said:

That's where they are. I am also trying 

$this->addHookAfter('Pages::save(template=TemplateName)', $this, 'afterSave');

but can't get it to work at the moment.

I already showed an example here https://processwire.com/talk/topic/21212-rockmigrations-easy-migrations-from-devstaging-to-live-server/?do=findComment&comment=212496 and here

https://processwire.com/talk/topic/25342-custom-classes-for-page-objects-the-discussion/?do=findComment&comment=212563

of course you can use $tpl = "template=yourtemplate" instead of using the class constant.

 

[MarkE]

OK - I think I may have this sorted:

1. The hooks must go in ___loaded(), __construct() is too early.
2. $this->addHookBefore('Pages::save(template=DbMigration)', $this, 'beforeSave'); seems to work OK

[MarkE]

OK @bernhard - looks like our posts crossed! _ I think I have it working, but your comments are helpful. I was tracking the other (coincidental) thread. Just to re-emphasise, __construct() is definitely the wrong place for this!!

[bernhard]

Are you sure you want to use Pages::save() ?

This does NOT mean that the page has been saved. For this we have the Pages::saved() hook! You are hooking into saving a single page field value, eg $page->save('myfield', 'myvalue');

[MarkE]

1 hour ago, bernhard said:

BTW I'd recommend doing this instead of $this != $p

There is a subtle but important difference between the above and your suggestion. Actually in this case, I needed $this != $p because I did not want the hook to run on any page other than the current instance. Generally, to filter the page classes, your suggestion is the right one. I am now doing the filtering with (template=...) as per my earlier post, so the filter in the method is no longer needed. However, as I have now discovered, for my use case, I do still need to check that the save being hooked is of the current instance - which can either be done by $this != $p or by using $p thoughout in the hook method (not sure which is better...) .

1 hour ago, bernhard said:

Are you sure you want to use Pages::save() ?

Good question! I am using the (before) hook to validate fields and so Pages::save() is right, I think.

[thetuningspoon]

I've taken to just creating a separate file called PageClassNameHooks.php for each PageClassName.php that needs hooks and then including it in init.php. Keeps things organized at least, and you can be sure they won't be initialized more than once.

[bernhard]

@thetuningspoon definitely a possible solution. Just wondering if you know about https://www.youtube.com/watch?v=eBOB8dZvRN4&t=517s and if so why you don't like/use that approach?

 

[thetuningspoon]

@bernhard This looks very cool. How does it know to only register the hook once when you're working with multiple pages of the same class?

[bernhard]

@thetuningspoon magicpages are super cool ? the only drawback is that they come with a small performance penalty as I need to load all available templates once on init and then attach the hooks or magic methods on applicable templates: https://github.com/baumrock/RockMigrations/blob/87541c899901f773bf62068a639f070f48a85453/MagicPages.module.php#L43-L51

The init() and ready() method (and the magic methods) are only called once for every template because I create one runtime page for each template on ProcessWire::init

The magic methods on the other hand only trigger for the correct pageclass because I have an early exit in the hook if the pageclass does not match: https://github.com/baumrock/RockMigrations/blob/87541c899901f773bf62068a639f070f48a85453/MagicPages.module.php#L113-L119

You might also like this:

 

[Jonathan Lahijani]

Has there been an updated, more formal, non-MagicPage approach to this yet?

Is there anything wrong with doing this so at least the hook is "closer" to the page class?

<?php namespace ProcessWire;

// this is /site/classes/OrderPage.php
// hook above class so it's "closer"

wire()->addHookAfter("Pages::saved(template=order)", function(HookEvent $event) {
  $order = $event->arguments('page');
  // hook code
});

class OrderPage extends Page {
  // ...
}

Related GitHub issue since I don't think it's linked in any of the replies above:
https://github.com/processwire/processwire-requests/issues/456

[Robin S]

Related tutorial:

 

[netcarver]

5 hours ago, Jonathan Lahijani said:

Is there anything wrong with doing this so at least the hook is "closer" to the page class?

This does introduce a side-effect to the class file. While it's not "wrong", it is discouraged, and will cause some code checkers to complain about the code. For example, if you are running codesniffer with one of the PSR standards, this would probably be flagged up (not checked.)

[bernhard]

23 minutes ago, netcarver said:

This does introduce a side-effect to the class file. While it's not necessarily "wrong", it is discouraged and will cause some code checkers to complain about the code. For example, if you are running codesniffer with one of the PSR standards, this would probably be flagged up.

Interesting idea @Jonathan Lahijani and thx for the input @netcarver! Is something like this also a problem? https://github.com/baumrock/RockFrontend/blob/107af5b51930d0589ecc0a882e46372b640eb6f9/RockFrontend.module.php#L26-L29

[netcarver]

@bernhard No, I think that's just a function declaration before a class declaration.  If in doubt, you can run phpcs on your code.  If you are only looking for side-effect warnings just grep your output for "effects"...

phpcs --standard=PSR12 file.php | grep "effects"

Run it without the grep for the full PSR12 ruleset...

phpcs --standard=PSR12 file.php

[netcarver]

@Jonathan Lahijani Please don't interpret what I wrote in reply to your post as negative - it's not meant to be. That may well be a fine solution for this. I appreciate knowing about the option, and having the link to the github issue you posted is a major bonus - I missed that one until now.

[iank]

On 5/14/2024 at 3:42 AM, Jonathan Lahijani said:

Has there been an updated, more formal, non-MagicPage approach to this yet?

An approach I sometimes use is the following.  It seems to work quite well and is fairly clean:

1. Put all your class-specific hooks in your page class, let's say in a ready() function (can be named anything of course, but ready() is a good convention for me).
2. In /site/ready.php, just instantiate a single instance of the page class and call its ready() method.

So, if we have let's say a NewsarticlePage class and we want to keep all news article related hooks together,  we might have something like this in the page class:

<?php

namespace ProcessWire;

class NewsarticlePage extends DefaultPage
{
	public function ready()
    {
        $this->addHook('ProcessPageEdit::buildForm', $this, 'addImportOptions');
        $this->addHookAfter("Pages::saved(template=newsarticle)", $this, 'processAfterSaveActions');
        $this->addHookAfter("Process::execute", $this, 'processAfterExecuteActions');
    }

And in site/ready.php:

<?php

namespace ProcessWire;

//required to trigger ready hooks in custom page classes:
(new NewsarticlePage())->ready();
(new SomethingElsePage())->ready();
//etc...

You can instantiate one empty page object for each Page class you need to run hooks for.  There's some overhead, but not much, and it keeps your site/ready.php clean and your hooks where they make sense.  Of course you still have to make sure the hooks only target the desired pages/templates.

Inspired by some of @bernhard's earlier posts on the subject.