Skip to content

Fix product attributes lookup data and filter by attribute widget with non-ASCII named attributes #34432

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.

Sign up for GitHub

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

Jump to bottom
Merged
Konamiman merged 4 commits into from
Dec 2, 2022
Merged

Fix product attributes lookup data and filter by attribute widget with non-ASCII named attributes #34432

Konamiman merged 4 commits into from
Dec 2, 2022

Conversation

@Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Aug 23, 2022

All Submissions:

Changes proposed in this Pull Request:

This pull request fixes three bugs related to the product attributes lookup table that were apparent when using an attribute with non-ASCII characters for variations:

  1. In PHP 8, saving a variable product that uses a non-ASCII attribute for variations throws a fatal error.
  2. An entry is generated in the lookup table for each term of the variation, not just for those actually used to define variations.
  3. Also the "Filter product by attribute" widget doesn't display.

The lookup table will require regeneration, at least for the affected products, after applying this fix.

How to test the changes in this Pull Request:

  1. Start with PHP 8 and without the fix.
  2. Make sure that you have product attributes lookup table enabled, including the option for direct updates (Settings - Products - Advanced).

image

  1. Create a global product attribute named 大きさ with these three terms: 中 大 小
  2. Add a "Filter by attribute" widget for the above in the shop.
  3. Create a variable product that uses that attribute for variations (with all three terms), but create only one variation for 大
  4. Save the product, you'll get a fatal error.
  5. Switch to PHP 7 and repeat the above.
  6. Create a second variable product that uses that attribute for variations, this time with only one variation for 小
  7. Go to the shop page, the filter by attribute widget is missing.
  8. Apply the fix.
  9. Reload the shop page, the widget is now visible but it displays incorrect information (as if the variations had been defined for "Any" value of the attribute):

FilterByJapaneseSize1

  1. Go to Status - Tools and run "Regenerate the product attributes lookup table", either for the entire table or for each of the products you just created.
  2. Reload the shop page, this time the widget shows correct information:

FilterByJapaneseSize2

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@Konamiman
Fix incorrect handling of non-ASCII attribute names for variations
0f076fa 
The issue manifested when the product attributes lookup table was enabled.

- In PHP 8, saving a variable product with an attribute whose name
  contains non-ASCII characters threw an error.
- In PHP 7, no error, but the lookup table was filled with entries
  for all the existing terms of the attribute, not just those
  used for variations. This resulted in the filter by attribute widget
  being rendered incorrectly.
@Konamiman
Add changelog file
19ba342
@Konamiman Konamiman self-assigned this Aug 23, 2022
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Aug 23, 2022
@Konamiman Konamiman requested review from a team and barryhughes and removed request for a team August 23, 2022 10:36
@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2022 •
edited
Loading

Test Results Summary

Commit SHA: cccad0c

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 58s
E2E Tests186006019213m 43s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@barryhughes
Copy link
Member

barryhughes commented Aug 23, 2022

Note to self: confirm if this also resolves #33971.

@barryhughes
Copy link
Member

barryhughes commented Aug 23, 2022

Hi @Konamiman 👋

I was able to replicate the problem and the fix works as described (no fatal error, and the attribute/term counts are accurate).

Non-ASCII filter: no results ASCII-range filter: has results
non-ascii ascii

☝🏽 However, as you can see in the above screenshot it appears that filtering doesn't work for these non-ASCII terms (see also the issue I referenced in my earlier comment).

  • Assuming you experience the same thing: should we address the above problem from within this PR, or is that out-of-scope? If it is out-of-scope, I wonder if we should be more specific about what we are fixing within the changelog?
  • What are your thoughts on an upgrade function that re-generates the product attributes lookup table when users next update their copy of WooCommerce?

@barryhughes
Copy link
Member

barryhughes commented Aug 23, 2022

Optional thought: thinking of the fatal error, I wonder if a safeguard should be applied here to check for WP_Errors?

@Konamiman
Copy link
Contributor Author

Konamiman commented Aug 24, 2022 •
edited
Loading

@barryhughes It's highly wtf that I didn't think of testing if the filters actually worked 🤦🏻‍♂️

I think that since one has the same root cause and it makes sense to fix it in this same pull request. As for the table regeneration on update, yes, I think we could do that only if there are references to non-ASCII attributes in the lookup table (it's as simple as checking if the table has values with % in the taxonomy field; if an attribute has a legitimate % as part of the name that would be a harmless false positive).

EDIT: The filters are working as expected for me. Does your url look like shop/?filter_大きさ=大&query_type_大きさ=or?

@Konamiman
Copy link
Contributor Author

Konamiman commented Aug 24, 2022

@barryhughes About the safeguard: that makes sense too, but I'm not sure what could we do in this case as the taxonomy is expected to exist at that point and if its id/slug pair can't be obtained then the entire feature is broken.

@Konamiman
Copy link
Contributor Author

Konamiman commented Aug 24, 2022 •
edited
Loading

I have found that the old WooCommerce "Filter by attribute" widget filters just fine, but the new blocks based widget not only doesn't, but also displays incorrect counts:

image

image

The incorrect count might to be caused by the fact that the blocks code is accessing the terms table directly, instead of doing wc_get_container()->get( Filterer::class )->get_filtered_term_product_counts as the old widget does. Disabling the lookup table usage causes the old widget to display wrong counts too.

As for the incorrect filtering, it's likely caused by how the request url is constructed:

Old widget: shop/?filter_大きさ=大&query_type_大きさ=or
Block: shop/?filter_大きさ=%25e5%25a4%25a7&query_type_大きさ=or

@barryhughes
Copy link
Member

barryhughes commented Aug 24, 2022

At this point I think we are happy to move forward without the migration and the further issue with the block-based widget is something we'll create a new issue for (so the relevant team can take a look at that).

On that basis, I think this is good to merge ... just pausing to check with you first of all (@Konamiman) before actually hitting 'merge' (to ensure my understanding is correct).

@Konamiman
Copy link
Contributor Author

Konamiman commented Aug 25, 2022 •
edited
Loading

@barryhughes I agree. The incorrect lookup table entries will affect only shops who meet all of:

  1. Use attributes with non-ASCII characters to define variations.
  2. Choose a set of terms from the attribute, but don't use all of them to actually create variations.
  3. Have "Hide out of stock items" setting set.

I'd say that while the number of affected shops might not be negligible, it's certainly not going to be the majority of them, so a migration for everyone could be conterproductive.

Anyway I'm merging from trunk to make the warnings about the Yodas disappear. 🙁 After that I'd say we're good to merge.

This was referenced Dec 11, 2023
Closed
Closed
@Konamiman Konamiman merged commit b7c5519 into trunk Dec 2, 2022
@Konamiman Konamiman deleted the fix/product-attributes-lookup-with-non-ascii-attributes branch December 2, 2022 08:54
@github-actions github-actions bot added this to the 7.3.0 milestone Dec 2, 2022
@Konamiman Konamiman mentioned this pull request Dec 2, 2022
Open
5 tasks
@github-actions github-actions bot mentioned this pull request Jan 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@barryhughes barryhughes barryhughes approved these changes

Assignees

@Konamiman Konamiman

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Konamiman @barryhughes