Skip to content

Conversation

ocean90
Copy link
Contributor

@ocean90 ocean90 commented Nov 14, 2018

  • Delete also expired site transients.
  • Add multisite support for site transients.
  • Ensure correct counts due to transient timeouts being its own option.
  • Add more tests for delete commands.

Fixes #30.
Fixes #41.

@ocean90 ocean90 changed the title [WIP] Improve deleting site transients Improve deleting site transients Nov 14, 2018
Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

Awesome work, @ocean90, looking good already!

I've written down a few remarks concerning the algorithm.

Also, I'd like to have the following tests added to the Behat suite as well:

  • deleting expired/all transients from site meta
  • deleting expired/all transients from multisite

(I notice now that the --expired is not tested at all, though...)

@schlessera
Copy link
Member

Another observation: I just saw that you've replaced esc_like() because it broke your tests on older WP core versions. We have a polyfill for that case that you can use instead: https://github.com/wp-cli/wp-cli/blob/v2.0.1/php/utils.php#L1408-L1424

@ocean90
Copy link
Contributor Author

ocean90 commented Nov 17, 2018

@schlessera After 90af941 three tests will fail because when deleting all transients the count is always one more. It seems to be that during each test WP cron is triggered which means _transient_doing_cron exists. Are you aware of this? Shouldn't cron be disabled like core is doing?

@schlessera
Copy link
Member

@ocean90 Oh, interesting. We should definitely disable cron for the tests, would also reduce the running time as well I guess.

@schlessera
Copy link
Member

@ocean90 I made a change to wp-cli/wp-cli-tests to disable WP Cron by default for running the tests: wp-cli/wp-cli-tests#28

Once this is merged, I'll tag a new release that you can then constrain the Composer file to for this package.

@ocean90
Copy link
Contributor Author

ocean90 commented Nov 17, 2018

@schlessera Thanks! I replaced FeatureContext.php locally and the tests are now passing. In terms of reducing the running time, it's actually the opposite. It's slower by ~3s. 🤷‍♂️

@schlessera
Copy link
Member

@ocean90 Release v2.0.11 of wp-cli/wp-cli-tests was now tagged. You can update the composer.json in your PR to require-dev ^2.0.11 of that package.

@schlessera
Copy link
Member

@ocean90 I felt the tests had become a bit haphazard now from the multiple changes.

I tried to restructure them to make them more consistent and to verify the expectations I would currently have of the logic.

With this change, I have the following two failures:

01. $ wp transient get foo3 --network

    Warning: Transient with key "foo3" is not set.
    cwd: /tmp/wp-cli-test-run-transient.feature.119-5bf661f2899b14.09807233/
    run time: 0.43985104560852
    exit status: 0
    In step `When I run `wp transient get foo3 --network`'.     # vendor/wp-cli/wp-cli-tests/features/steps/when.php:29
    From scenario `Deleting expired transients on single site'. # features/transient.feature:119
    Of feature `Manage WordPress transient cache'.              # features/transient.feature

02. $ wp transient delete --expired --network
    Success: No expired transients found.

    cwd: /tmp/wp-cli-test-run-transient.feature.277-5bf6620361a5a2.52144689/
    run time: 0.43504905700684
    exit status: 0
    In step `Then STDOUT should be:'.                           # vendor/wp-cli/wp-cli-tests/features/steps/then.php:17
    From scenario `Deleting expired transients on multisite'.   # features/transient.feature:277
    Of feature `Manage WordPress transient cache'.              # features/transient.feature

According to my current understanding, this would point to issues with the code, nto the tests, but I might be missing something obvious (it's early in the morning, after all).

Can you look into the tests to help me find out whether my expectations or the code are off (we can discuss in Slack if you prefer)?

@schlessera schlessera added this to the 2.0.1 milestone Nov 22, 2018
@schlessera schlessera merged commit 9e7a786 into wp-cli:master Nov 22, 2018
@schlessera
Copy link
Member

Thanks for the great contribution, @ocean90 !

@ocean90 ocean90 deleted the fix/delete-all-transients branch November 22, 2018 18:39
schlessera added a commit that referenced this pull request Dec 23, 2021
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.

2 participants