Lint the various modules #734
Conversation
… correctly due to spacing problems
…ormatted correctly
| } | ||
| // Check if name field was filled in | ||
| if ( empty( $status_name ) ) { | ||
| $change_error = new WP_Error( 'invalid', esc_html__( 'Please enter a name for the status.', 'edit-flow' ) ); |
There was a problem hiding this comment.
Nit: Escaping HTML in an error isn't usually necessary, especially as we escape it in the output line below. I assume this came from a linting error though, so maybe it made you do that.
There was a problem hiding this comment.
(same for the 5 WP_Error/die()s below)
There was a problem hiding this comment.
It's def the linter forcing me to do it, and I have the same qualm with it - why do it if it's being escaped already at the bottom? I wanted to reduce the amount of ignores that I had in place too tbh, so I just went ahead and did it
| set_current_screen( 'edit-custom-status' ); | ||
| $wp_list_table = new EF_Custom_Status_List_Table(); | ||
| $wp_list_table->prepare_items(); | ||
| echo wp_kses_post( $wp_list_table->single_row( $return ) ); |
There was a problem hiding this comment.
Just noting - wp_kses_post() on a table row seems odd. Ideally $wp_list_table->single_row() should return sanitized HTML (it doesn't) and we should ignore the warning here. However, I tested this (happens during quick edits on the EditFlow Admin -> Custom Statuses page, then hover over a status and click "Quick Edit" and then "Update Status"), and it seems to work totally fine.
There was a problem hiding this comment.
Yip, agreed on that. That's how I had tried it, but the resulting ignore wherever this table class is used wasn't too well liked by me. I'd like to minimize the ignores so I ended up doing this.
That said, I'm thinking that we can see during our feature development if we are going to keep this class or do it differently perhaps.
modules/dashboard/dashboard.php
Outdated
| <h4><a href="<?php echo $url ?>" title="<?php _e('Edit this post', 'edit-flow') ?>"><?php echo $title; ?></a></h4> | ||
| <span class="ef-myposts-timestamp"><?php _e('This post was last updated on', 'edit-flow') ?> <?php echo get_the_time('F j, Y \\a\\t g:i a', $post) ?></span> | ||
| </li> | ||
| <h4><a href="<?php echo esc_url( $url ); ?>" title="<?php _e( 'Edit this post', 'edit-flow' ); ?>"><?php echo esc_html( $title ); ?></a></h4> |
There was a problem hiding this comment.
| <h4><a href="<?php echo esc_url( $url ); ?>" title="<?php _e( 'Edit this post', 'edit-flow' ); ?>"><?php echo esc_html( $title ); ?></a></h4> | |
| <h4><a href="<?php echo esc_url( $url ); ?>" title="<?php esc_attr_e( 'Edit this post', 'edit-flow' ); ?>"><?php echo esc_html( $title ); ?></a></h4> |
| // or that the term name doesn't map to an existing term's slug | ||
| $search_term = $this->get_editorial_metadata_term_by( 'slug', sanitize_title( $metadata_name ) ); | ||
| if ( is_object( $search_term ) && $search_term->term_id != $existing_term->term_id ) { | ||
| $change_error = new WP_Error( 'invalid', esc_html__( 'Name conflicts with slug for another term. Please choose again.', 'edit-flow' ) ); |
There was a problem hiding this comment.
Double-escape here, can use use __() and let die() escape below
There was a problem hiding this comment.
This one's the same as custom status - it's due to the linting rules raising a concern here. Would rather not have ignores all over so I just ended up double escaping
| */ | ||
| // Check if name field was filled in | ||
| if ( empty( $name ) ) { | ||
| $change_error = new WP_Error( 'invalid', esc_html__( 'Please enter a name for the user group.', 'edit-flow' ) ); |
There was a problem hiding this comment.
| $change_error = new WP_Error( 'invalid', esc_html__( 'Please enter a name for the user group.', 'edit-flow' ) ); | |
| $change_error = new WP_Error( 'invalid', __( 'Please enter a name for the user group.', 'edit-flow' ) ); |
There was a problem hiding this comment.
This one's like this for the same reason as the other modules. The linter forces it and I'd rather not have ignores all over the place
| } | ||
| // Check that the name doesn't exceed 40 chars | ||
| if ( strlen( $name ) > 40 ) { | ||
| $change_error = new WP_Error( 'invalid', esc_html__( 'User group name cannot exceed 40 characters. Please try a shorter name.' ) ); |
There was a problem hiding this comment.
| $change_error = new WP_Error( 'invalid', esc_html__( 'User group name cannot exceed 40 characters. Please try a shorter name.' ) ); | |
| $change_error = new WP_Error( 'invalid', __( 'User group name cannot exceed 40 characters. Please try a shorter name.' ) ); |
There was a problem hiding this comment.
(same for the two below)
There was a problem hiding this comment.
This one's like this for the same reason as the other modules. The linter forces it and I'd rather not have ignores all over the place
There was a problem hiding this comment.
@ingeniumed Thank you so much for doing these changes! There were a ton of really valuable updates to escaping functions, translator strings, and isset checks, as well as a lot of code-quality updates like adding braced and correcting code spacing. I found turning whitespace off (settings gear -> hide whitespace) in the diff was very necessary for review. Note I also tested features manually in this branch to ensure nothing obvious was broken, and everything loaded and ran well.
I left comments on specific issues I noticed above.
There was one big change we may want to reconsider (mentioned once here), and that's the switch from date() to gmdate(). This was done in a lot of places. gmdate() is the correct function to use generally speaking, but these functions return different values.
Unless we're really careful to migrate date calculations correctly, this could cause scheduled posts and other date-related features (like the calendar) to suddenly change and be off by a few hours. I think users typically expect WordPress to reflect the date and timezone set in Admin -> Settings -> General, and modifying our core date function without careful testing could cause unexpected behavior in multiple features.
I don't know of any specific place of where this will break, but I think I could find some if you'd like a more concrete example.
What do you think?
Thanks for reviewing it, this isn't an easy PR to review so I appreciate it. I think out of safety, I'll revert that change and exclude that rule from phpcs ignore for now. In a follow up PR, we can do rigorous testing and get that in. While I did test it briefly, and it worked well I don't think we should risk letting it go in in this giant PR. You are 100% right on it imo |
| @@ -836,7 +836,7 @@ public function get_notification_footer( $post ) { | |||
| $body .= sprintf( __( 'You are receiving this email because you are subscribed to "%s".', 'edit-flow' ), ef_draft_or_post_title( $post->ID ) ); | |||
| $body .= "\r\n"; | |||
| /* translators: 1: date */ | |||
| $body .= sprintf( __( 'This email was sent %s.', 'edit-flow' ), gmdate( 'r' ) ); | |||
| $body .= sprintf( __( 'This email was sent %s.', 'edit-flow' ), date( 'r' ) ); | |||
There was a problem hiding this comment.
Realized this was switched in a previous PR, so I have reverted it.
Description
This PR is meant to lint all the PHP classes at once to make development easier. I have skipped the commons folder, just like we skipped it for the JS linting in the interest of not breaking everything. We should revisit that decision or try to refactor it, but I'll set that aside for now.
There's also a bug fix here as the settings updated css wasn't being applied correctly. That's resolved now.