processwire
"noHooks" option for Page::save() doesn't call Pages::save() and Pages::saveField() with underscores
Short description of the issue
Related forum topic: https://processwire.com/talk/topic/25850-nohooks-option-in-page-save/
The documentation for the options argument of Page::save() just refers to the documentation for Pages::save(), and here the explanation of the noHooks option is pretty brief:
noHooks (boolean): Prevent before/after save hooks (default=false), please also use $pages->___save() for call.
It doesn't explain exactly which hookable methods related to saving pages are affected by the noHooks option. But it does make clear that you have to call $pages->___save() with underscores or else hooks to Pages::save() will still fire (which the user probably doesn't want if they are specifying the noHooks option). A similar note exists in the documentation for Pages::saveField().
But the issue is that if you are calling $page->save(['noHooks' => true]) or $page->save('some_field', ['noHooks' => true]) then this method doesn't include the underscore prefix when it calls Pages::save() or Pages::saveField() so some save-related hooks will still fire.
https://github.com/processwire/processwire/blob/f851c638427cf362bddab7f21529c426a13fb2b8/wire/core/Page.php#L2773-L2780
Some thoughts...
1. When the noHooks option is true then Page::save() should call Pages::___save()/Pages::___saveField() with underscores.
2. In the documentation it would be good to include a list of hookable methods that are affected by the noHooks option.
3. The documentation for Page:save() should probably include it's own separate description of the noHooks option rather than just refer to the docs for Pages::save() because two extra methods are involved, namely Pages::save() and Pages::saveField().
Setup/Environment
- ProcessWire version: 3.0.181
@Toutouwai I checked and the noHooks option indicates that you have to call it via $pages->___save(), so I feel like the docs are already clear about that, at least about how to call it. Though they aren't clear about if you should ever consider using this option. If this were a common or useful option for people to use, then sure, we'd want to facilitate it further. But in this case, this is an option that I don't think anyone should actually use, or at least it should be really rare. It was added for a specific and rare core purpose a long time ago, but I'm not sure that one even applies anymore. Maybe it should be removed or at least undocumented, as preventing hooks from running can be problematic, so we'd rather people didn't do it. For instance, Fieldtypes may use hooks to handle completion of a task at the right time, or to do cleanup. File fields are a good example. So preventing hooks could leave things undone in some cases. Is this an option that you need to use for something? If you can tell me more about it, maybe I should suggest other ways to do it.
I used noHooks in my ProcessDbMigrate module, which is where I came across the issue. The forum topic referenced above has more details. ProcessDbMigrate is currently at the proof of concept stage, but I have done a lot of work on it recently (not yet on GitHub) and it is looking more releasable. IIRC it uses noHooks in 2 places for very specific reasons, which might be addressed a different way. However I am away from my desk (and IDE) for a week, so can’t recall the exact reasons nor explore alternatives. I will do so on my return and report back.
Just for the record: I've used noHooks numerous times, though whether it's strictly speaking necessary is another question. I could probably work out around it, but if it was removed, it might break a few of my modules.
Basically I've always assumed this was the preferred approach if a save should occur that doesn't trigger hooks, including those cases where you're saving content within a save hook (in which case it might cause an infinite loop). And yes, at least for me "save that doesn't trigger any hooks" seems to be a pretty common need. Perhaps it has something to do with large number of my modules being somehow related to logging, versioning, or duplicating/stashing content :)
Real life examples:
- https://github.com/teppokoivula/SearchEngine/blob/master/lib/Indexer.php#L122
- https://github.com/teppokoivula/VersionControl/blob/dev/VersionControl.module#L315
But the issue is that if you are calling $page->save(['noHooks' => true]) or $page->save('some_field', ['noHooks' => true]) then this method doesn't include the underscore prefix when it calls Pages::save() or Pages::saveField() so some save-related hooks will still fire.
Just a few days ago I was wondering why SearchEngine index field operations get logged into VersionControl history, even though they were not supposed to. This explains it. 🤦
Now that I've re-read the documentation and the comment above from Ryan I kind of get it (and it looks like I should use another method instead of Pages::save()), but I do still feel that this is a bit unintuitive.
- When the noHooks option is true then Page::save() should call Pages::___save()/Pages::___saveField() with underscores.
+1. Can't really see any downsides to this.
I checked and the noHooks option indicates that you have to call it via $pages->___save(), so I feel like the docs are already clear about that, at least about how to call it.
Right, but as described in the original post noHooks is also stated as an option for Page::save, and yet this method itself doesn't follow the recommendation of calling $pages->___save() when noHooks is true.
Apologies, I'm still trying to understand, so bear with me. The Page::save() method documentation says this: See Pages::save() documentation for options. So if you do that, and look at the noHooks option it says: please use $pages->___save() for call. Why would I expect it to work for $page->save() when the doc is stating you call it from $pages and that you prepend the 3 underscores to the method call? ? The method docs are for people that want to use the options, and not for the little man living in the Page class. :) The way I see it, this option is one that probably nobody other than you guys should be using anyway (and perhaps should be an undocumented one), so adding code to facilitate its usage elsewhere and to a broader audience doesn't seem right to me. Though if there is a reason why you would need to use this option through $page->save() that can't be accommodated by the the way it's supposed to be called with $pages->___save() then let me know and in that case we should add it. Or if you think the existing documentation needs clarification, perhaps replacing the "please" with a "you must" seems appropriate?
Now I understand what the documentation intended, I suggest that the solution is simple. Remove the reference to options in Page:save() and leave it as it is in Pages:save(). Alternatively, in Page:save() just document the options that are intended to be used with it. The problem is that the current documentation could easily be interpreted as meaning that the three underscores are only required when using the noHooks option with Pages:save() and that noHooks works OK with just plain $page->save(). It’s a bit like one of those optical illusions with two images 😉
@MetaTunes Of the things you mentioned, I think probably copying the supported options to the Page::save() is best, as it would prevent the confusion on that noHooks option, as well as save people the trouble of having to click through to another method to find the options.
The logical solution to me is updating Page::save to something like the below (there are different ways you could add the conditional):
public function save($field = null, array $options = array()) {
if(is_array($field) && empty($options)) {
$options = $field;
$field = null;
}
if(empty($options['noHooks'])) {
if(empty($field)) {
return $this->wire('pages')->save($this, $options);
}
if($this->hasField($field)) {
return $this->wire('pages')->saveField($this, $field, $options);
}
} else {
if(empty($field)) {
return $this->wire('pages')->___save($this, $options);
}
if($this->hasField($field)) {
return $this->wire('pages')->___saveField($this, $field, $options);
}
}
// save only native properties
$options['noFields'] = true;
return $this->wire('pages')->save($this, $options);
}
That way there's no need to treat noHooks as a special case that must be excluded from $options argument of Page::save that will in turn be passed to Pages::save / Pages::saveField.
But seeing as this issue has never affected me personally I'm happy to go with the flow.
I am content with @ryancramerdesign ‘s solution. From what I know, that will leave an undocumented use of noHooks in Page:save(), where the effect of the option is only to not run hooks on Page:save() itself, leaving all hooks on downstream methods such as in Pages. Up to @ryancramerdesign whether to document that. I think it best not to alter the existing behaviour, given that it is not a bug, as there is an outside chance it might break someone’s site.
I would consider the solution suggested by @Toutouwai preferable: the amount of added complexity seems pretty minimal, and this would make the behaviour of Page::save() more consistent with Pages::save().
Obviously not a huge deal though — I just fail to see a good reason why page::save shouldn't support noHooks if it could, especially if that would simultaneously (at least in my opinion and experience so far) make things ever so slightly more predictable.
I think it best not to alter the existing behaviour, given that it is not a bug, as there is an outside chance it might break someone’s site.
I can see the logic here, but at the same time I don't fully agree: if someone is currently using an option that isn't doing what they think it does, seems more likely that this change would fix existing issue(s) rather than break things :)
I'm confused. What does the noHooks option actually do, if it is required that you call the save method with the triple underscores as well? Doesn't calling a method directly with the triple underscores bypass the hooks for that method anyway?
I've used the noHooks option within beforeSave hooks before to try to avoid an infinite loop, and I thought it worked. But apparently I've been using it wrong because I never noticed the bit about how it only works when calling Pages::___saved() with the triple underscores. Could have sworn that note wasn't there last time I looked...!
I also didn't find it totally obvious that this option would not work with Page::save(), even with that note. I thought I might be able to do $page->___save(['noHooks' => true]), but that proved fruitless.
EDIT: I think I understand now. noHooks prevents the other hooks called within Pages::save() from firing, like Pages::saveReady(). This would also explain why it has worked for me before in the past, because usually we hook saveReady instead of save directly for before hooks.
Just commenting here to say I hit this issue today as well and was wondering why noHooks was firing when in my mind it shouldn't have.
