Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$template->childTemplates & $template->parentTemplates not updated when template is deleted #802

Open
adrianbj opened this issue Jan 29, 2019 · 18 comments

Comments

@adrianbj
Copy link

@adrianbj adrianbj commented Jan 29, 2019

Short description of the issue

If you delete a template that has been added to another template's childTemplates array, it is not removed from this array

Expected behavior

It should be automatically removed.

Actual behavior

It isn't removed.

Optional: Screenshots/Links that demonstrate the issue

Your screenshots/links go here.
image

Steps to reproduce the issue

  1. Add template to childTemplates array for another template
  2. Delete the added template from the system
@ryancramerdesign
Copy link
Member

@ryancramerdesign ryancramerdesign commented Feb 4, 2019

Unless it's resulting in a visible error somewhere (?), this one is expected, and not a problem. When references are encoded (rather than existing in their own DB column), it's not necessary (and sometimes not practical) that they be removed until the next time the value holding them is modified and/or saved.

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Feb 5, 2019

I don't remember if I came across an error, but I probably did or I wouldn't have noticed this discrepancy. In a couple of my modules I do checks on whether there a template has childTemplates and I believe these would be incorrect with the current behavior. I use modules from other devs that also do this check.

I noticed in ProcessPageListerPro.module there is:

if(empty($template->childTemplates)) continue;

Maybe I am misunderstanding something, but won't this return false rather than true in the situation I have described. It seems to on my isolated tests. Maybe there are some other checks in there that I haven't noticed that are taking care of updating the childTemplates property?

@ryancramerdesign
Copy link
Member

@ryancramerdesign ryancramerdesign commented Feb 14, 2019

The line(s) you mentioned in Lister/ListerPro are just optimizations to skip over an additional check if the value happens to be empty. There are many instances where ProcessWire core as well as both core and 3rd party modules keep track of these kinds of settings in encoded values. All configurable modules and fields work in this way as well. The childTemplates setting on templates is just one of potentially hundreds. Settings like this are adjusted at the time the object that encodes the setting is saved. It wouldn't be practical or efficient otherwise. When we need a setting that is automatically adjusted in the manner you described, we reserve a dedicated DB column or table for it, which is what enables it to be efficiently managed in that way.

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Feb 14, 2019

So if a module author want to get an accurate result for whether a template has a particular template in its childTemplates, what is the recommended way to check this? Do we need to check the childTemplates property and then do an additional check to see if each of the returned IDs are actually valid templates?

@ryancramerdesign
Copy link
Member

@ryancramerdesign ryancramerdesign commented Feb 14, 2019

So if a module author want to get an accurate result for whether a template has a particular template in its childTemplates, what is the recommended way to check this?

Maybe something like this below (?)

function templateHasChildTemplate(Template $template, Template $childTemplate) {
  return in_array($childTemplate->id, $template->childTemplates); 
}

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Feb 14, 2019

Ok, sorry, I guess that was the wrong question because in your example the template has to exist or the function will throw an error.

I am talking about a situation where you want to know if a template has any childTemplate restrictions and currently it will return the IDs of templates which may no longer exist and the count will be wrong.

I guess I am talking a relatively uncommon need. I just don't like that there is incorrect data stored.

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Feb 20, 2019

@ryancramerdesign - I have actually come across an error with this. This line: https://github.com/processwire/processwire/blob/649d2569abc10bac43e98ca98db474dd3d6603ca/wire/modules/PagePermissions.module#L849 causes a:

PHP Notice: Trying to get property 'useRoles' of non-object

Note that the notice doesn't show for superusers so that might confuse you at first.

@adrianbj adrianbj reopened this Feb 20, 2019
@adrianbj
Copy link
Author

@adrianbj adrianbj commented Mar 13, 2019

@ryancramerdesign - actually, it's possible for the error to show for superusers. I am seeing this with Tracy's RequestInfo panel - it makes a call to see if the page has addable() permission and if you have one of these deleted childTemplates, it gives you that "Trying to get property 'useRoles' of non-object" error.

@Toutouwai
Copy link

@Toutouwai Toutouwai commented Mar 13, 2019

I believe my suggested fix in #543 would resolve this issue too. Specifically this addition:

if(!$template) continue; // childTemplates may contain IDs for templates since deleted

@netcarver
Copy link
Collaborator

@netcarver netcarver commented Mar 14, 2019

@ryancramerdesign Bumping this for your consideration.

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Oct 8, 2019

@ryancramerdesign - I am still getting the PHP Notice: Trying to get property 'useRoles' of non-object notice when logged in as a non-superuser. Is there any chance of revisiting this please?

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Oct 11, 2019

@netcarver - could you please remove the "not a bug" tag? Maybe that will get Ryan's attention again. My logs are being polluted with:
image

Thanks!

@netcarver
Copy link
Collaborator

@netcarver netcarver commented Oct 12, 2019

@ryancramerdesign Still seems to be an issue.

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Dec 3, 2019

OK, I finally decided to deal with the error messages myself because it was getting a bit silly :)

This script makes it quick and easy to figure out which templates have childTemplates values that include templates that no longer exist:

image

Of course this could easily be adjusted to automatically remove 79 from the childTemplates setting for the "sheeps" template, but I decided to do it manually via Adminer.

Anyway, it would be nice if when a template is deleted, other templates were scanned for its ID in their childTemplates setting and remove it. I am almost tempted to hook into Templates::delete and take care of it myself, but it really does belong in the core.

@BernhardBaumrock
Copy link

@BernhardBaumrock BernhardBaumrock commented Dec 4, 2019

@adrianbj what do you think of adding such fixes to a dedicated module? We could have fixes in place quickly (and publicly) and @ryancramerdesign could easily test those fixes and add them to the core. I've created a module for this: https://github.com/BernhardBaumrock/PwCoreFixes

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Dec 5, 2019

Hi @BernhardBaumrock - I like your proactive approach to the PwCoreFixes module, but I fear that it might actually slow down fixing these things because if users start adopting it, they'll be less likely to add their voice to the actual issue reports and so Ryan won't see that the issue affects other users as well. I hope I am wrong though and I really appreciate your effort!

Here is a hook based fix for cleaning up both the child and parent templates when a template is deleted. Feel free to add it to your module if you'd like.

$this->addHookBefore('Templates::delete', function($event) {
    $templates = $event->object;
    foreach($templates as $t) {

        // child templates
        $childTemplates = $t->childTemplates;
        if($childTemplates) {
            foreach($childTemplates as $i => $tid) {
                if(!$templates->get($tid)) {
                    unset($childTemplates[$i]);
                    $t->childTemplates = $childTemplates;
                    $t->save();
                }
            }
        }

        // parent templates
        $parentTemplates = $t->parentTemplates;
        if($parentTemplates) {
            foreach($parentTemplates as $i => $tid) {
                if(!$templates->get($tid)) {
                    unset($parentTemplates[$i]);
                    $t->parentTemplates = $parentTemplates;
                    $t->save();
                }
            }
        }
    }
});

@adrianbj adrianbj changed the title $template->childTemplates not updated when template is deleted $template->childTemplates & $template->parentTemplates not updated when template is deleted Dec 6, 2019
@adrianbj
Copy link
Author

@adrianbj adrianbj commented Dec 7, 2019

Just an FYI about that code example above - obviously that will do a complete cleanup of all orphaned templates in childTemplates and parentTemplates, rather than the one that was just deleted. Probably not the approach that the core should adopt, but it's nice for this fix in that it cleans up existing problems as well as future ones.

@adrianbj
Copy link
Author

@adrianbj adrianbj commented Jun 20, 2020

@ryancramerdesign - can you please reconsider this. I am still getting this error:

ErrorException: Trying to get property 'useRoles' of non-object in wire/modules/PagePermissions.module:849

and it's pretty clear why :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants