-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PLANET-4626 Update composer dev dependencies to match blocks-plugins, #1005
Conversation
ac3ba7f
to
5c7a89e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you bring this up to date with the changes from the latest release? I suggest to not try fix the merge conflicts as too much changed inside those files, and just use the version from the release and fix any violations it has.
Besides that and a couple smaller remarks the PR looks good.
tests/p4-testcase.php
Outdated
} | ||
|
||
|
||
/** | ||
* Use wp unit testcase factories to create data in database for the tests. | ||
*/ | ||
function initialize_planet4_data() { | ||
public function initialize_planet4_data() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be private as it's only for internal use. Maybe we can add a check in the linter to see if public functions are actually used outside the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a linter check on public function usage would be nice idea, if you have some link/details in this context would be very helpful 👍
8beeef9
to
329d660
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few small questions.
classes/class-p4-master-site.php
Outdated
$options = get_option( 'planet4_options' ); | ||
$weight = get_post_meta( $post->ID, 'weight', true ); | ||
$options = get_option( 'planet4_options' ); | ||
$default_max_wt = P4_Search::DEFAULT_MAX_WEIGHT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since P4_Search::DEFAULT_MAX_WEIGHT
is a constant there really isn't a need to put it in a variable.
classes/class-p4-campaigns.php
Outdated
</th> | ||
<td> | ||
<?php wp_dropdown_pages( $dropdown_args ); ?> <?php echo $redirect_edit; ?> | ||
<p class="description"><?php echo __( 'Leave this empty if you want to use the automated Tag page. Otherwise pick a page to redirect this Tag to.', 'planet4-master-theme-backend' ); ?></p> | ||
<?php wp_dropdown_pages( $dropdown_args ); ?> <?php echo $redirect_edit; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be escaped?
Sync phpcs configuration with blocks-plugins, Fix PHPCS errors
329d660
to
ca07dea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Sync phpcs configuration with blocks-plugins,
Fix PHPCS errors
JIRA 4626