Add support for other post types #223 #342
Conversation
…the core plugin file
.gitignore
Outdated
| .svn | ||
| composer.lock | ||
| vendor/ | ||
| /nbproject |
There was a problem hiding this comment.
I recommend that you add the /nbrpoject to your global gitignore, that way you won't have to add it to the project. Or you can move it to where the .idea folder is ignored
liveblog.php
Outdated
| * This allows the users to filter their supported post types. | ||
| */ | ||
| add_post_type_support( 'post', self::key ); | ||
| self::$supported_post_types = apply_filters( 'modify_supported_post_types', $post_types ); |
There was a problem hiding this comment.
Filters have to be prefixed. Some useful info here.
liveblog.php
Outdated
| */ | ||
| public static function is_viewing_liveblog_post() { | ||
| return (bool) ( is_single() && self::is_liveblog_post() ); | ||
| return (bool) ( is_singular() && self::is_liveblog_post() ); |
There was a problem hiding this comment.
This will return true for all post types, including unsupported post types. This needs more precision. Also, possibly a filter - not sure.
There was a problem hiding this comment.
is_singular will return true for all default single post types.
Correct me if I am wrong, but for it to return true on a custom post type, the name of the custom post type needs to be sent as a parameter.
Also, how do you think this conditional should be handled?
There was a problem hiding this comment.
There was a problem hiding this comment.
is_singular by default will check if any post type is singular, not just post or pages. So you have to check the is_signular() against all of the post types that support LIveblog.
liveblog.php
Outdated
| */ | ||
| public static function get_liveblog_state( $post_id = null ) { | ||
| if ( ! is_single() && ! is_admin() && ! self::$is_rest_api_call) { | ||
| if ( !is_singular() && ! is_admin() && ! self::$is_rest_api_call) { |
There was a problem hiding this comment.
Same as above - needs more precision.
There was a problem hiding this comment.
I think we could use get_post_type() here.
https://developer.wordpress.org/reference/functions/get_post_type/
Something like:
...
if ( !is_singular( get_post_type( $post_id ) ) ... )
There was a problem hiding this comment.
is_singular will accept post types as the parameter. You can pass the static $supported_post_types variable to the function instead.
| public static $auto_archive_days = null; | ||
| public static $auto_archive_expiry_key = 'liveblog_autoarchive_expiry_date'; | ||
|
|
||
| public static $supported_post_types = array(); |
There was a problem hiding this comment.
Why is a static variable better here than a regular class variable?
There was a problem hiding this comment.
The init method is static. As far as I know PHP doesn't allow non-static members to be accessed in a static method.
There was a problem hiding this comment.
Ahh, you're right - the is_liveblog_post() method is static too and it's going to rely on this.
| * This allows the users to filter their supported post types. | ||
| */ | ||
| self::$supported_post_types = apply_filters( 'modify_supported_post_types', $post_types ); | ||
| self::$supported_post_types = apply_filters( 'liveblog_modify_supported_post_types', $post_types ); |
There was a problem hiding this comment.
Can you remove the modify from the filter? I think that would be better because it would almost look like the variable name we're already using: liveblog_supported_post_types
|
Changes looking good! I think we need just a few more tweaks and we're good to go. |
|
Hey @justnorris, I was wondering if you had a chance to look at the tests that are failing. I could use a little help there |
|
Have a look at the test logs - could it be that your tests issue is related to #344 ? |
|
It could be. |
|
In that case, I think we can wait until the tests are pulled in the master branch - then you can update your pull request to make sure the tests pass. |
|
That sounds good. Thanks, Norris! |
|
Hey @justnorris! |
|
You can pull the master branch in your pull request branch - that should update everything and make the tests run again. |
|
I think this looks good! /cc @philipjohn |
|
This PR has a merge conflict. |
There was a problem hiding this comment.
Thanks for this @shantanu2704! Could you take a look at my one inline comment and the merge conflict please? Then I think we can include this in v2.0 :)
| * we can possibly introduce this to other post types later. | ||
| * Add liveblog support to the 'post' and 'page' post type. | ||
| */ | ||
| $post_types = array( 'post', 'page' ); |
There was a problem hiding this comment.
If I understand correctly, this will also add support for pages out of the box? I'd rather we just stick to posts, and allowing pages to be added using the filter.
|
Hey @philipjohn! Thanks for the feedback! Closing out this PR in favour of #539 :) |
Added a static variable for supported post types that can be filtered with the 'modify_supported_post_types' filter in the init() method.
Added support for 'pages' in the init() method.