Skip to content

Conversation

@htdat
Copy link
Member

@htdat htdat commented May 12, 2021

Fix #639

Description

Details here #639 (comment)
This PR should be merged after this PR #643 - actually, this PR is rebased on PR 643.

Steps to Test

  1. Check out PR.
  2. Set up WP test site bash bin/install-wp-tests.sh wordpress_test root root.
  3. Run PHPUnit tests in PHP 8.0.x composer integration.
  4. Confirm that all tests pass.

@htdat htdat requested review from mikeyarce, natebot and nielslange May 12, 2021 01:50
@htdat htdat self-assigned this May 12, 2021
@mikeyarce
Copy link
Contributor

Looking at the code, it looks like this is set to be (int) here:
https://github.com/Automattic/Edit-Flow/blob/124c3cde900c22abe15c04b0b2bd11e306b2aef4/modules/story-budget/story-budget.php#L201

However, in the test it doesn't look like we're casting as (int) here:
https://github.com/Automattic/Edit-Flow/blob/2e07a1b7b7af1e783b858700390da85b418c2405/tests/test-edit-flow-story-budget.php#L68

Instead of changing the code, would it make more sense to update the test to cast as (int)?

@htdat
Copy link
Member Author

htdat commented Jun 1, 2021

@mikeyarce

Looking at the code, it looks like this is set to be (int) here:
https://github.com/Automattic/Edit-Flow/blob/124c3cde900c22abe15c04b0b2bd11e306b2aef4/modules/story-budget/story-budget.php#L201

This is a good point!

However, in the test it doesn't look like we're casting as (int) here:
https://github.com/Automattic/Edit-Flow/blob/2e07a1b7b7af1e783b858700390da85b418c2405/tests/test-edit-flow-story-budget.php#L68
Instead of changing the code, would it make more sense to update the test to cast as (int)?

L68 here is already an int so I don't think we need to cast it.

However, actually, #639 mentions the error in other lines and test case:
https://github.com/Automattic/Edit-Flow/blob/2e07a1b7b7af1e783b858700390da85b418c2405/tests/test-edit-flow-story-budget.php#L88-L92

We can get rid of this test case but I think it's still good to keep this test and practice defensive coding for the EF_Story_Budget::update_user_filters_from_form_date_range_change method as it can be used somewhere else in the future?

Copy link
Contributor

@mikeyarce mikeyarce left a comment

Choose a reason for hiding this comment

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

After discussion, this sounds like a good approach!

@htdat htdat merged commit 2642a91 into master Jun 1, 2021
@htdat htdat deleted the fix/phpunit-php8 branch June 1, 2021 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PHP 8.0 Plugin Compatibility - test_story_budget_set_number_days_filter_invalid

3 participants