godaddy-wordpress / sake

Our internal build tool for WP/WC plugins
MIT License
4 stars 0 forks source link

Update search:wt_update_key #66

Closed unfulvio closed 3 years ago

unfulvio commented 3 years ago

Summary

Updates the search:wt_update_key to not produce errors if a plugin does no longer use the woothemes_queue_update but has a valid Woo header with product Id and key for the Woo updater.

Story: MWC 1644

QA

Unfortunately the regex doesn't seem to work for me... But it's a bit strange, because I tested it into https://regex101.com/ and produces the expected results there...

Setup

Follow these steps to run a local copy of Sake using this branch.

Test this locally and try to launch the sake search:wt_update_key on

The tests should include the following outcomes (note that all plugins use currently both these methods so to test things out you may need to remove one or the other conditionally):

Steps

woothemes_queue_update plugin and no header

* Woo header plugin and no php function

Open questions

Are we ok with this the type of regex proposed?

unfulvio commented 3 years ago

@adrianostanley

Plugins implementing the woothemes_queue_update() are expected to pass since the regex is the same as before.

Plugins that removed the woothemes_queue_update() function, and have the * Woo: <prodId>:<prodKey> set are failing to me but I'm not sure why since the regex worked in simulations in regex tools / ide.

I am not aware of plugins that don't implement any of these, except the one not expected to be deployed on WooCommerce.com, but the point of the test in QA was to verify that a plugin expected to be deployed on WooCommerce.com lacking any of these values should fail.

I took woocommerce-checkout-add-ons (main branch which still the woo-includes folder) and changed the Woo: 466854:8fdca00b4000b7a8cc26371d0e470a8f number and no matter what I put here in order to break it, the regex accepts it... example Woo: @@:@@@! Shouldn't it error out?

There isn't much of validation in either regex other than the following

data.match(/\s*\*\s*Woo:\s*\d*:(.+);/ig)

Should match <space><asterisk><space>Woo:<space><any digit>:<any alphanumeric string>

the digit (product ID) can have any length, so other than checking for a number I don't think there's anything else we can parse

as for the product key (the alphanumeric string) that is an hash, I'm not sure if its length is supposed to be variable or fixed, but the current way the old method parses it should be the same (.+) so I wanted to leave that unchanged.

  • Woo header plugin and no php function Can you give me a plugin example that uses it this way? Sorry, but I didn't understand what's "Woo header plugin and no php function". Thank you!

The current main branch of Local Pickup Plus I wanted to release yesterday has this. I couldn't release it because of Sake hit an error. I could deploy by forcing Sake to pass, but wanted to fix the issue nonetheless.

Otherwise, you can simulate this with any plugin and just remove the old woothemes updater reference. We are in the process of removing that for plugins, so eventually more deployments will hit the same problem.

unfulvio-godaddy commented 3 years ago

@adrianostanley hey thanks for trying again - hey did it actually work for you? because if I test this PR on a branch that does not have woothemes_queue_update() it will fail. I don't know why the regex isn't matching. In simulators it works. The pattern is similar as the regex for matching woothemes_queue_update. If you test this branch on plugins that still use woothemes_queue_update it will work because that hasn't changed. I don't think this can be merged until both modes work. Could you confirm you were able to run this using a local instance of Sake and on a repository that did have woothemes_queue_update in the main file? Thanks!

unfulvio commented 3 years ago

@adrianostanley finally it worked for me as well, it was an accidental semicolon in the matcher https://github.com/skyverge/sake/pull/66/commits/e432cb0234b406bb4918b3977d03f12125f1b54e