Skip to content

Fixes PHP notices related to comments#386

Merged
donnchawp merged 2 commits into
Automattic:masterfrom
stodorovic:fix/385_PHP_notices_related_to_comments
Oct 3, 2017
Merged

Fixes PHP notices related to comments#386
donnchawp merged 2 commits into
Automattic:masterfrom
stodorovic:fix/385_PHP_notices_related_to_comments

Conversation

@stodorovic
Copy link
Copy Markdown
Contributor

Fixes #385

@stodorovic
Copy link
Copy Markdown
Contributor Author

stodorovic commented Sep 14, 2017

@donnchawp I'm not sure that's the best way, but it seems as readable code without a lot of changes:

defined( 'DONOTDELETECACHE' ) or define( 'DONOTDELETECACHE', 1 );

Related to if ( $_GET[ 'delete_all' ] != 'Empty Trash' && $_GET[ 'delete_all2' ] != 'Empty Spam' ), it's pretty outdated. It depends on language:

French - $_GET[ 'delete_all' ] = "Vider la corbeille"

WP uses (from 3.0 to 4.8) this condition:

if ( isset( $_REQUEST['delete_all'] ) || isset( $_REQUEST['delete_all2'] ) )

@stodorovic
Copy link
Copy Markdown
Contributor Author

For first iteration, I decided to add at the begin of function wp_cache_get_postid_from_comment:

if ( defined( 'DONOTDELETECACHE' ) ) {
        return;
}

Comment thread wp-cache-phase2.php
global $super_cache_enabled, $wp_cache_request_uri;

if ( defined( 'DONOTDELETECACHE' ) ) {
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Return the post_id?

@donnchawp
Copy link
Copy Markdown
Contributor

Looks great and definitely improves that function, but it should return the post ID those two times it returns early.

@donnchawp donnchawp added this to the 1.5.7 milestone Oct 2, 2017
@stodorovic
Copy link
Copy Markdown
Contributor Author

Function wp_cache_get_postid_from_comment is triggering only from couple actions.

wp-cache-phase2.php:  add_action('trackback_post', 'wp_cache_get_postid_from_comment', 99);
wp-cache-phase2.php:  add_action('pingback_post', 'wp_cache_get_postid_from_comment', 99);
wp-cache-phase2.php:  add_action('comment_post', 'wp_cache_get_postid_from_comment', 99);
wp-cache-phase2.php:  add_action('edit_comment', 'wp_cache_get_postid_from_comment', 99);
wp-cache-phase2.php:  add_action('wp_set_comment_status', 'wp_cache_get_postid_from_comment', 99, 2);

I've checked it with xdebug and it seems that this function only define constant DONOTDELETECACHE. I don't see the code which uses post_id which is returned from function. Maybe we can remove all values from return. Also we could move this function into new function wpsc_...... (then we can add deprecation notice for current function if other plugin uses it).

https://developer.wordpress.org/reference/hooks/trackback_post/
https://developer.wordpress.org/reference/hooks/pingback_post/
https://developer.wordpress.org/reference/hooks/comment_post/
https://developer.wordpress.org/reference/hooks/edit_comment/
https://developer.wordpress.org/reference/hooks/wp_set_comment_status/

I just checked again and all hooks are actions. So, it makes sense to we totally remove return post_id (and probably reorganize the code) in the future. We should think over about it.

Anyway, I'll try to add post_id now to we keep compatibility with legacy code.

Comment thread wp-cache-phase2.php
) {
wp_cache_debug( "Delete all SPAM or Trash comments. Don't delete any cache files.", 4 );
define( 'DONOTDELETECACHE', 1 );
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Return the post_id?

@donnchawp
Copy link
Copy Markdown
Contributor

You're right! Thanks for checking. I think that function should be renamed in the future so it's obvious it's not returning something. I had forgotten that.

I'll merge this code as-is. We can see about further improvements later. Thanks!

@donnchawp donnchawp merged commit aad054b into Automattic:master Oct 3, 2017
@stodorovic stodorovic deleted the fix/385_PHP_notices_related_to_comments branch December 17, 2017 17:58
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.

2 participants