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

Fix Courses page throwing 404 in Hello Elementor theme #7683

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Sep 9, 2024

Resolves #7678

Proposed Changes

When using the Hello Elementor Theme, our Courses page (/courses-overview) threw a 404.

To explain the reason, we should know that Sensei handles the Courses archive page very differently than usual.

When a request comes to render the Course archive page, we override the usual flow of the request, and handle it using this class. From an archive request, we change it to a single post request, so every plugin or theme after that point considers and handles the request as a single page request. And, as the global single post to be rendered, we set a dummy post, the content of which is whatever we want to render, In this case, a custom content we generate by rendering the blocks from the page from "Settings -> General -> Course Archive Page" setting.

The issue here is not because of the content, but rather, because of the properties of the global wp_query to go with the single post we set. Notice that we set up all the properties to imitate that of a single page, is_singular and is_page is set to true and is_archive is set to false.

Now check out this code in Hello Elementor. This is where it decides which template to choose. Notice that, to render the single template, it requires the is_singular check to be true. But even though we set is_single, we don't set is_singular to true. Which causes the is_singular check to fail. Thus, it falls back to the 404 template. So, setting is_singular to true fixes the archive rendering issue, and Hello Elementor starts using the single template as we want.

Notice that we've also removed queried_object_id. It's because once we set "is_singular" to true, this WP Core check also starts passing. So it tries to fetch the dummy post again from the database using get_post() using the id from the queried_object_id, where it doesn't exist. So it gets a null object, and subsequently throws an error. So I've removed the queried_object_id to prevent it from happening.

Testing Instructions

  1. Install the "Hello Elementor" theme.
  2. Go to the Courses archive page
  3. Make sure it renders without any issue
  4. Add a couple of courses to a category
  5. Check the category Course list from frontend to make sure it doesn't throw any error when the archive is rendered under taxonomy (One way to render courses under category is to click on the category tag that's shown on each course in the course list)
  6. Now create a copy of the Courses page, give it a different name and permalink. Set this new pages as the course archive page from "Settings -> General -> Course Archive Page"
  7. Try to access the new page and make sure it doesn't throw a 404
  8. Test the above steps in all top themes to make sure no regression has happened.
  9. Try to think about any scenario where it can cause a regression.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@Imran92 Imran92 added this to the 4.24.4 milestone Sep 9, 2024
@Imran92 Imran92 requested a review from a team September 9, 2024 18:37
@Imran92 Imran92 self-assigned this Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Sep 9, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Sep 9, 2024

Test the previous changes of this PR with WordPress Playground.

@donnapep donnapep changed the title Fix Courses page throwin 404 in Hello Elementor theme Fix Courses page throwing 404 in Hello Elementor theme Sep 9, 2024
@donnapep
Copy link
Collaborator

donnapep commented Sep 11, 2024

@Imran92 This seems to work. 🙂 Curious if you tracked down the original PR where this change was introduced to gain an understanding of why it was added in the first place and ensure that we didn't reintroduce a bug? I did some testing based on the original PR and everything seems to work fine. 🤞🏻

Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but I did add a comment.

@Imran92
Copy link
Contributor Author

Imran92 commented Sep 12, 2024

@Imran92 This seems to work. 🙂 Curious if you tracked down the #3704 where this change was introduced to gain an understanding of why it was added in the first place and ensure that we didn't reintroduce a bug? I did some testing based on the original PR and everything seems to work fine. 🤞🏻

Hi Donna! Thank you so much for checking the PR and testing for possible unidentified side effects so thoroughly.

Yap I did check all the previous relevant PRs. The PR that first introduced this issue was this one actually. I think we just needed the queried_object to be set, not the queried_object_id. That's why it caused some notice issue that were visible on the Taxonomy pages. Which you fixed with this PR, by preventing setting the queried_object_id on tax pages. My PR can be considered just an extension of the same PR which just fixes the issue for non-tax pages as well.

@donnapep donnapep merged commit f634821 into trunk Sep 13, 2024
22 checks passed
@donnapep donnapep deleted the fix/archive-404-issue-in-hello-elementor branch September 13, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Hello Elementor makes the Courses page return 404
2 participants