Add theme compatibility for Module page

[alexsanford]

Partial solution to #2154

Adds theme compatibility for the Module page. A few notes:

• I refactored the request handlers so that each page is handled by a different class. This way Sensei_Unsupported_Themes won't get out of hand.

• The "Dummy Post" may not be immediately clear. It's adapted from WooCommerce. In essence we do the following:

  • Render the Module page to a string using the template.
  • Create a dummy post (of type "page") with enough information to be rendered.
  • Set the dummy post's content to the rendered Module page string.
  • Tell WordPress to render the dummy post using one of the available templates.

• Unlike Add theme compatibility for Course Page #2155, I opted not to use a Renderer class. It simplified the logic, especially because within the handler, we already know that the globals are all set up for rendering the Module template. In the Renderer, we wouldn't necessarily have that context. If we need to render the Module page in other contexts in the future, we can extract the logic then.

Still to do

• Write tests

Testing

• Create a few modules with some Lessons and a description.

• From the Course page, click on one of the Module titles.

• Ensure that the page looks right on themes that support Sensei, and themes that do not support Sensei.

• For themes that do not support Sensei, ensure that:

  • The title is not duplicated.
  • The pagination shows up properly at the bottom.

[jom]

Code looks good! One issue I'm running into is with Sensei compatible widgets. I think this is probably caused by the manipulation of the main loop's WP_Query. I'm not sure if it is possible to clone $wp_query and restore it after the_content renders in time for the template/widget area render?

Using the theme Astra, here is a screenshot of a module page on master:

Same page on add/theme-compat-module-page:

(missing the Course Progress widget)

[donnapep]

@alexsanford Are there more changes coming as per #2205 (comment), or can I go ahead and test?

[alexsanford]

@donnapep I will be adding changes to fix that issue. Hopefully today.

[alexsanford]

@jom I set up a fix for this.

I originally tried resetting the globals ($wp_query and $post) after the loop completed. The problem is that the theme I was testing on (Astra) got tripped up when the query was different before and after the loop. In particular, it sets up conditional wrapper logic, which was adding a closing 'div' without the opening one since is_archive was false before the loop and true afterward (see https://github.com/brainstormforce/astra/blob/master/inc/class-astra-loop.php#L246).

As a result, I feel that the best solution may be to leave globals set as they are so that the theme renders properly, but to change the globals when rendering a dynamic sidebar. This way, the theme thinks it is rendering a page the whole time (i.e. the dummy page that we set up in code), but the sidebar widgets think that the current query is for a module.

I do worry that there may be other places in code where things may need to check if we're on a module page. Currently, the globals are set to the module query for the content, and the widgets. I can't think of another place that may have 3rd-party code that might need to make this check, but let me know if there's somewhere else I should test.

[donnapep]

Looks like there's a problem with the module progress indicator for the Divi theme. If I've completed a lesson in one of the modules, before this change I see the progress indicator styled like so:

After this PR (or more likely after #2155), I see an unstyled progress indicator:

[alexsanford]

@donnapep Yes, that CSS was broken by this PR.

I've put together a solution, but I'm a bit concerned about some of the implications of this issue.

The first problem was that the is_tax method was not returning true. Turns out this was an easy fix involving the $wp_query object, and I made the required change in 29ba305. Also I needed to remove the dependence on #main in the CSS; see b1ddd39.

After fixing is_tax, my thought was that this could also be the proper solution for the problem that Jake found earlier in the Course Progress widget (see #2205 (comment) and #2205 (comment)).

However when I tried to revert the commit that solved that issue previously, I found that along with using is_tax, the Course Progress extension is also expecting the global $post object to be a Lesson. This is not the case when we're rendering the dummy post. In other words, just fixing is_tax isn't enough.

So I did not revert the commit that fixed the widget. Essentially, this is the implication:

• When rendering a dynamic sidebar with widgets, $wp_query and $post will be set as they would normally be when rendering a Module page on a supported theme (i.e. everything should work).

• When rendering the module page template to get the HTML for the content area, $wp_query and $post will also be set as they normally would (because we render this into a string before the dummy post is set up).

• Outside of those two above cases, is_tax will return true, but $post will not be a Lesson. Meaning that any code running in some action or filter that falls outside of the two cases above (e.g. in a 3rd party extension) will not be able to determine the current Lesson/Course using the $post object. For example, see how Course Progress uses it here.

My concern is that this could potentially cause really weird issues that are going to be hard to track down, and it may not be obvious that it's a theme incompatibility issue or that adding compatibility to the theme will fix it.

[donnapep]

I'm a bit concerned about removing the #main selector. Is this element completely removed? It seems as though it may be safer to leave it in, and then add new selectors that exclude #main.

[alexsanford]

Is this element completely removed?

It depends on the theme, but in general, yes.

It seems as though it may be safer to leave it in, and then add new selectors that exclude #main.

Hmm. I see what you're getting at, but I don't think this would make any difference, technically. For example, the selector .module-archive #main .status, .module-archive .status will match the exact same elements as .module-archive .status.

Removing the #main selector effectively makes the rules less specific. I.e. they will still match all the elements that would have been matched previously, but they will match other elements as well.

So the potential problem here is not that elements will be missed, but that we will match elements that we don't intend to. The rules are all scoped to .module-archive or .course, which gives us some safety. But technically, the possibility exists. We might be able to make the rules more specific, even after removing #main. Do you think that's worth looking into?

[donnapep]

Nah, we'll likely uncover this during testing, if a problem exists.

[donnapep]

Outside of those two above cases, is_tax will return true, but $post will not be a Lesson. Meaning that any code running in some action or filter that falls outside of the two cases above (e.g. in a 3rd party extension) will not be able to determine the current Lesson/Course using the $post object.

So this change will break Course Progress by not showing it on the module page at all, and will potentially break third party code. Is that right? If that's the case, it sounds like we may need to rethink this approach and see if we can find an alternative that doesn't cause those side effects.

Let's start by seeing if @jom has any ideas.

[alexsanford]

@donnapep

So this change will break Course Progress by not showing it on the module page at all, and will potentially break third party code. Is that right?

Almost 🙂

This will not break Course Progress. I specifically added a fix for this issue that will apply within dynamic sidebars, and so this will not be an issue for widgets. That's the fix in 25a556f.

As a contrived example of how this could break, let's say an extension added a filter to body_class in order to add the class .module-course-<course_id> to the body on all module pages. They might do so with code like so (untested):

function sensei_add_module_course_body_class( $classes ) {
	global $post;

	if ( is_tax( 'module' ) ) {
		$lesson_id = $post->ID;
		$course_id = get_post_meta( $lesson_id, '_lesson_course', true );

		$classes[] = "module-course-$course_id";
	}

	return $classes;
}

add_filter( 'body_class', 'sensei_add_module_course_body_class' );

The problem is that because this code runs outside of the module content rendering and outside of a dynamic sidebar, this will have different behaviour for a supported theme and an unsupported theme:

• On a supported theme, the code will behave as expected.

• On an unsupported theme, is_tax( 'module' ) will behave as expected, but the global $post will not be the first lesson in the module. Instead, it will be the dummy post that we create. Thus, its ID will be 0 and $course_id will likely be null.

I honestly don't know whether this is actually going to be a problem for our users. It may be purely theoretically. WooCommerce apparently didn't run into it, but I don't know whether that's a good indicator for whether we should worry about it.

[donnapep]

I honestly don't know whether this is actually going to be a problem for our users. It may be purely theoretically.

It's been my experience that anything theoretical will come back to bite you in the butt. 😆It seems risky enough to spend some time trying to find a solution, but probably not worth spending more than a day on it.

[alexsanford]

After a long chat with @jom about this, we were not able to come up with a good technical solution. The problem, in short, is that we need the global $post to be set to the dummy WP_Post object in order for the theme's template to render properly. Thus, in general, code that expects $post to be a Sensei CPT on the Module page will fail.

That said, this problem is scoped to the following conditions:

• The user is running an unsupported theme.
• The user is using 3rd-party code that expects $post to be a Sensei CPT while on the Module page.
• The access of $post occurs within an action or filter that runs outside of the following two contexts:
  1. The render of the Module taxonomy template
  2. A dynamic sidebar (i.e. within a widget)

Our proposed solution is to leave this as-is, and ensure that we are communicating the risk appropriately through our theme support documentation. Here are some more things that we could do which may help:

• When supporting users, ask them to test on a supported theme (to rule out these kinds of issues).
• Add default support within Sensei for popular themes (here is a proof-of-concept).
• [Optional] Create functions for 3rd-party developers to give them the information that they may try to access through $post (e.g. the current Course for the Module page). This may help, but would not be a complete solution (since we will not be able to ensure that all 3rd-party code uses the functions).
• [Optional] If we determine some areas where this could commonly come up, we can add custom fixes for those areas (like we did for dynamic sidebars).

cc @donnapep

[donnapep]

@alexsanford OK, thanks for the investigation, and I like your recommendations. Let's take this on a case-by-case basis if / when it comes up.

[jom]

Thanks for your work on this!

[donnapep]

👍

[alexsanford]

I chatted with @dbtlr about this, and his suggestion was that adding functions to access the Sensei object (Lesson, Course, etc) would be a better option moving forward, rather than having the specific handling in place for dynamic sidebars.

Since this work as-is now, I'm going to merge it in order to unblock further work. However, I've opened an issue (#2233) to discuss the option of adding the functions and removing the dynamic sidebar handling. Let's continue the conversation there.