Make WordPress Core

Opened 10 months ago

Closed 8 months ago

Last modified 7 months ago

#58345 closed defect (bug) (fixed)

Critical error if page_for_posts is set to non existing page ID

Reported by: wplindavantol's profile wplindavantol Owned by: azaozz's profile azaozz
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch
Focuses: Cc:

Description

Description
If page_for_posts points to a non-existing page ID, get_posts() will fail and cause a critical error in line 406 (nav-menu.php). So probably a check is needed to prevent this. Tested with default theme and no active plugins.

Critical error location
Dashboard --> Appearance --> Menus

Error log

PHP Fatal error:  Uncaught Error: Attempt to assign property "posts_page" on null in /.../wp-admin/includes/nav-menu.php:406
Stack trace:
#0 /.../wp-admin/includes/template.php(1577): wp_nav_menu_item_post_type_meta_box()
#1 /.../wp-admin/nav-menus.php(984): do_accordion_sections()
#2 {main}
   thrown in /.../wp-admin/includes/nav-menu.php on line 406, referer: https://.../wp-admin/nav-menus.php? 
   action=locations

nav-menu.php

<?php
  // Insert Posts Page.
     $posts_page = 'page' === get_option( 'show_on_front' ) ? (int) get_option('page_for_posts' ) : 0;

     if ( ! empty( $posts_page ) ) {
           $posts_page_obj             = get_post( $posts_page );
           $posts_page_obj->posts_page = true;

           $important_pages[]   = $posts_page_obj;
           $suppress_page_ids[] = $posts_page_obj->ID;
     }

Attachments (1)

patch-58345-2.patch (1.3 KB) - added by hbhalodia 9 months ago.
Patch for ticket - 58345

Download all attachments as: .zip

Change History (17)

#1 @SergeyBiryukov
10 months ago

  • Keywords php8 added
  • Milestone changed from Awaiting Review to 6.3

This ticket was mentioned in PR #4472 on WordPress/wordpress-develop by @hbhalodia.


10 months ago
#2

  • Keywords has-patch added

#3 @hbhalodia
10 months ago

Hi @SergeyBiryukov, As per @wplindavantol description, I tried to reproduce the same error on my system. I got to reproduce the same error. What I followed is to provide the non-existing ID for this part of code logic and successfully encountered the error. Though I have added the patch for this ticket, I am bit confuse here,

  • I have tried to reproduce the error by setting the homepage and posts page in settings→reading and then deleted the page which was assigned to posts page. When I revisited the menu, it defaults taken the next page ID but does not add the error.
  • I think when we intentionally add the non-exisiting ID, then only the error is reproducible.

@wplindavantol Can you please provide the steps how you have reporoduced the error without intenionally passing the non-existing Id. Thanks.

Ps: Though I have added the patch, here is the PR for the same - https://github.com/WordPress/wordpress-develop/pull/4472

#4 @wplindavantol
10 months ago

@hbhalodia The problem existed before I was asked to look into it. I tried to reproduce it with a possible scenario, but I couldn't reproduce it.

I have created a new user. With this user I created a page, which I set as a 'blog' page. Then I deleted this user, without linking the content to another user. But that didn't cause this problem.

#5 @hbhalodia
10 months ago

Yes @wplindavantol, By default I was also not able to reproduce the issue without explicitly adding the non-existing ID. When I add any ID which I know that is not exists, it's reproducing that way only.

#6 @josklever
10 months ago

When I investigated this issue together with @wplindavantol we've found other occurrences of this issue.

So it's probably a good idea to check if the post exists and if not set the value to 0, like:

if ( is_null($posts_page_obj) ) { update_option('page_for_posts', 0); }

Disclaimer: I'm not a developer, so this needs to be checked/tested.

#7 @audrasjb
9 months ago

Thanks for the patch @hbhalodia however there's a logic issue in the PR: if $front_page_obj or $posts_page_obj is null we should fallback to the else statement.

#8 @audrasjb
9 months ago

  • Version trunk deleted

#9 @audrasjb
9 months ago

  • Keywords changes-requested added

#10 @hbhalodia
9 months ago

@audrasjb,

Got it, $front_page_obj = get_post( $front_page );, this check needs to be checked in the first if statement. If any of the condition fails should be fallbacking to the else statement. Something like below,

if ( ! empty( $front_page ) ) {
    $front_page_obj = get_post( $front_page );
}

if ( $front_page_obj ) {
    $front_page_obj->front_or_home = true;

    $important_pages[]   = $front_page_obj;
    $suppress_page_ids[] = $front_page_obj->ID;
} else {
 ....
}

And for the $posts_page_obj there was no default else statement previously. Should we need to add the same as we added for $front_page_obj.

Let me know so that I can edit the patch correctly.

Thanks.

#11 @audrasjb
9 months ago

Yeah this is exactly what I meant 👍

#12 @hbhalodia
9 months ago

Thanks, @audrasjb Would updated the patch accordingly. I am adding the patch file here and if looks good, would update the PR too.

@hbhalodia
9 months ago

Patch for ticket - 58345

#13 @oglekler
8 months ago

  • Keywords needs-testing added; changes-requested removed

New patch needs testing.

#14 @azaozz
8 months ago

  • Keywords php8 needs-testing removed

patch-58345-2.patch makes sense and seems to work here. As @hbhalodia I'm also a bit surprised about how these two options can point to non-existing posts, but seems it is not impossible :)

#15 @azaozz
8 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 56236:

Menus: Fix critical errors when the page_on_front and/or page_for_posts options contain references to non-existing posts.

props: hbhalodia, wplindavantol, josklever, audrasjb.
Fixes: #58345.

This ticket was mentioned in Slack in #forums by josklever. View the logs.


7 months ago

Note: See TracTickets for help on using tickets.