Fix/phpcs #286

oscarssanchez · 2022-08-04T20:20:05Z

Description of the Change

This PR fixes PHPCS issues detected as well as adding a new PHPCS check

How to test the Change

Changelog Entry

Added - New feature
Changed - Existing functionality
Deprecated - Soon-to-be removed feature
Removed - Feature
Fixed - Bug fix
Security - Vulnerability

Credits

Props @username, @username2, ...

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests pass.

felipeelia

@oscarssanchez left some comments throughout the PR, let me know if you have any questions. Congrats on the huge effort here!

felipeelia · 2022-08-12T11:35:27Z

.github/workflows/lint.yml

+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '7.2'


@oscarssanchez I imagine you are not using the default PHP 8.1 because it is still incompatible with the codebase. Can we bump this to PHP 7.4 at least?

Bumped here: db2d1d9

felipeelia · 2022-08-12T11:39:08Z

includes/admin/api/class-bc-admin-media-api.php

-				if ( isset( $_POST['folderId'] ) && isset( $_POST['oldFolderId'] ) ) {
-					$folderId    = sanitize_text_field( $_POST['folderId'] );
-					$oldFolderId = sanitize_text_field( $_POST['oldFolderId'] );
+				if ( isset( $_POST['folder_id'] ) && isset( $_POST['oldfolder_id'] ) ) {
+					$folder_id     = sanitize_text_field( $_POST['folder_id'] );
+					$old_folder_id = sanitize_text_field( $_POST['oldfolder_id'] );


@oscarssanchez I am probably missing it but did you also change the place where oldFolderId is sent in the POST request?

You are right @felipeelia , I've fixed it here: 552364b

felipeelia · 2022-08-12T11:40:39Z

includes/admin/api/class-bc-admin-media-api.php

-		while ( count( $results ) <= ( $page * $posts_per_page ) ) {
+		$count_results = count( $results );
+
+		while ( $count_results <= ( $page * $posts_per_page ) ) {


As $results is changed inside the while, are you sure this will keep the same behavior it had before?

This code is actually not used anymore, so I just removed the whole method db94cfc

felipeelia · 2022-08-12T11:41:21Z

includes/admin/api/class-bc-admin-media-api.php

-		$folder_id = ( isset( $_POST['folderId'] ) && '' !== $_POST['folderId'] ) ? sanitize_text_field( $_POST['folderId'] ) : false;
+		$folder_id = ( isset( $_POST['folder_id'] ) && '' !== $_POST['folder_id'] ) ? sanitize_text_field( $_POST['folder_id'] ) : false;


Same as before: did we also change the place where that POST field is sent?

felipeelia · 2022-08-12T11:42:19Z

includes/admin/api/class-bc-admin-media-api.php

+	 * @param string $account_hash The account hash for the account.
+	 * @param int    $video_id     The ID of the video to associate the image with.
+	 * @param string $url         The URL of the image to upload.
+	 * @param int    $width       The width of the image.
+	 * @param int    $height     The height of the image.


Can we align these descriptions? :)

Fixed here 1069e77

felipeelia · 2022-08-12T11:47:06Z

includes/admin/class-bc-admin-menu.php

+		 *
+		 * This hook doesn't follow standard naming convention but needs to stay as it is for retro compatibility.
 		 */
-		do_action( 'brightcove/admin/settings_page' );
+		do_action( 'brightcove/admin/settings_page' ); // phpcs:ignore


Can't we mark this as deprecated with do_action_deprecated and add a properly named action?

Fixed here ff0df76

felipeelia · 2022-08-12T11:48:11Z

includes/admin/class-bc-admin-settings-page.php

-				echo $this->render_source_rows();
+				echo $this->render_source_rows() // phpcs:ignore


This should keep the ; (this is a good example of why ignoring specific rules can help :P )

Fixed here af798f7

felipeelia · 2022-08-12T11:51:08Z

includes/api/class-bc-cms-api.php

+		 * name strings or quoted strings
+		 * text strings or quoted strings (name, description, long_description)
+		 * tags strings or quoted strings
+		 * reference_id string or quoted string
+		 * state    ACTIVE, INACTIVE, DELETED, PENDING
+		 * updated_at   date range
+		 * created_at   date range
+		 * schedule.starts_at   date range
+		 * schedule.ends_at date range
+		 * published_at date range
+		 * complete true or false


It would be great if we could align these descriptions here too. I think that would increase readability by a ton. Thoughts?

Fixed here ab24ae0

felipeelia · 2022-08-12T11:52:43Z

includes/api/class-bc-oauth.php

-	public function _request_access_token( $force_new_token = false, $retry = true ) {
+	public function request_access_token( $force_new_token = false, $retry = true ) {


I don't know how many people would use or extend this code but we should mark this as a breaking change OR keep the old version of the method marking it with _doing_it_wrong or similar.

Fixed here 31b09b8

felipeelia · 2022-08-12T12:05:49Z

includes/class-bc-setup.php

+					$message = sprintf(
+						'<div class="error"><p><strong>%1$s</strong>%2$s<strong>%3$s</strong>%4$s %5$s %6$s<strong>4.2</strong></p></div>',
+						esc_html__( 'Brightcove Video Cloud Enhanced', 'brightcove' ),
+						esc_html__( 'has been', 'brightcove' ),
+						esc_html__( 'deactivated', 'brightcove' ),
+						esc_html__( 'because it\'s incompatibale with WordPress version', 'brightcove' ),
+						esc_html( get_bloginfo( 'version' ) ),
+						esc_html__( 'The minimum compatible WordPress version is ', 'brightcove' )
+					);


Unfortunately, breaking up strings to make parts translatable makes translators work much harder. WP core does not care about leaving  in the string to be translated, for example.
A suggestion here would be something like:

$message = sprintf( /* translators: 1. WP version; 2. Minimum compatible WP version */ __( 'Brightcove Video Cloud Enhanced has been deactivated because it is incompatible with WordPress version %1$s. The minimum compatible WordPress version is %2$s', 'brightcove' ), esc_html( get_bloginfo( 'version' ) ), '4.2' ); echo wp_kses_post( '<div class="error">' . $message . '</div>' );

This is also not used anymore so It was removed here 8b6cce4

felipeelia

lgtm, just remember to state in the changelog that some methods had their names changed @oscarssanchez

oscarssanchez added 30 commits August 1, 2022 13:56

Initial PHPCS checks

b184483

PHPCBF fixes

6692501

brightcove-video-connect.php phpcs fixes

65171b2

cli/class-brightcove-cli.php phpcs fixes

015d546

class-bc-admin-media-api.php phpcs fixes

26a646f

class-bc-admin-labels-page.php phpcs fixes

3fea880

class-bc-admin-menu.php phpcs fixes

cf0b1c3

class-bc-admin-playlists-page.php phpcs fixes

d7a48b4

class-bc-admin-settings-page.php

3e70d61

class-bc-admin-sources.php phpcs fixes

3c7f09c

class-bc-admin-user-profile.php phpcs fixes

53ca539

class-bc-admin-videos-page.php phpcs fixes

5943efe

class-bc-templates.php phpcs fixes

d29cde5

class-bc-api.php phpcs fixes

feae4b8

class-bc-cms-api.php phpcs fixes

051658b

class-bc-experiences-api.php phpcs fixes

594c612

includes/api/class-bc-oauth.php phpcs fixes

31e1a98

includes/api/class-bc-player-management-api.php phpcs fixes

b2976a1

includes/api/class-bc-player-management-api2.php phpcs fixes

898975c

includes/api/class-bc-text-track.php phpcs fixes

e022ca6

includes/class-bc-accounts.php phpcs fixes

dad8c04

includes/class-bc-errors.php phpcs fixes

67da919

includes/class-bc-experiences-shortcode.php phpcs fixes

df0fd2b

includes/class-bc-in-page-experience-shortcode.php phpcs fixes

9808c30

includes/class-bc-labels.php phpcs fixes

75d141b

includes/class-bc-logging.php phpcs fixes

b196c2e

includes/class-bc-notification-api.php phpcs fixes

e662a3f

includes/class-bc-permissions.php phpcs fixes

0004fcd

includes/class-bc-playlist-shortcode.php phpcs fixes

3888ac5

includes/class-bc-setup.php phpcs fixes

e7bf58f

oscarssanchez added 7 commits August 4, 2022 15:18

includes/class-bc-utility.php phpcs fixes

40352ad

includes/class-bc-video-shortcode.php phpcs fixes

39dc9d9

includes/class-bc-video-upload.php phpcs fixes

f6ca93b

includes/sync/class-bc-playlists.php phpcs fixes

1728cf2

includes/sync/class-bc-videos.php phpcs fixes

295fc2c

uninstall.php phpcs fixes

d01be57

Merge branch 'develop' into fix/phpcs

456df46

oscarssanchez requested review from felipeelia and tott August 9, 2022 22:52

felipeelia reviewed Aug 12, 2022

View reviewed changes

oscarssanchez added 15 commits September 4, 2022 19:42

Fixes activation error

82b69f1

Fixes bug with old folder ID update

552364b

Bump php lint version used

db2d1d9

Remove unused code

db94cfc

Align descriptions

1069e77

Align descriptions

9158805

Deprecate actions

ff0df76

Align docs

5234eb4

Specify phpcs rule

ae933bc

Align docs

ab24ae0

Add comma

af798f7

Remove unused code

8b6cce4

Add phpcs rule

b261726

Deprecate method

31b09b8

PHPCS fixes

454cb98

oscarssanchez requested a review from felipeelia October 3, 2022 16:36

felipeelia approved these changes Oct 3, 2022

View reviewed changes

oscarssanchez merged commit 8af718a into develop Oct 4, 2022

oscarssanchez deleted the fix/phpcs branch October 4, 2022 16:21

oscarssanchez mentioned this pull request Oct 4, 2022

Update documentation from PHPCS fixes #294

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/phpcs #286

Fix/phpcs #286

oscarssanchez commented Aug 4, 2022 •

edited

Loading

felipeelia left a comment

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022 •

edited

Loading

felipeelia Aug 12, 2022

oscarssanchez Oct 3, 2022

felipeelia left a comment

		$folder_id = ( isset( $_POST['folderId'] ) && '' !== $_POST['folderId'] ) ? sanitize_text_field( $_POST['folderId'] ) : false;
		$folder_id = ( isset( $_POST['folder_id'] ) && '' !== $_POST['folder_id'] ) ? sanitize_text_field( $_POST['folder_id'] ) : false;

		echo $this->render_source_rows();
		echo $this->render_source_rows() // phpcs:ignore

		public function _request_access_token( $force_new_token = false, $retry = true ) {
		public function request_access_token( $force_new_token = false, $retry = true ) {

Fix/phpcs #286

Fix/phpcs #286

Conversation

oscarssanchez commented Aug 4, 2022 • edited Loading

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

felipeelia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarssanchez Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeelia left a comment

Choose a reason for hiding this comment

oscarssanchez commented Aug 4, 2022 •

edited

Loading

oscarssanchez Oct 3, 2022 •

edited

Loading