Skip to content
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

Use Fewer post meta values #578

Closed

Conversation

pattonwebz
Copy link
Member

@pattonwebz pattonwebz commented Apr 8, 2024

This PR aims to reduce the number of different post meta values in favor of storing as much data inside of a single post meta as possible.

The exception is that for sorting by columns some meta still must exist in 2 places.

It reduces the total number of post meta by 5 and introduces a single interface to getting any piece of post meta.

https://github.com/equalizedigital/accessibility-checker/pull/578/files#diff-ac3197d4f34d0b1b8fafdf64365bd3e7cb32db3aa83571ea74fcdfe464adf0eeR118-R122

Closes: #402

If we have no key to save to and the data is not an array then return without updating anything since something is wrong.
@SteveJonesDev
Copy link
Member

@pattonwebz, here is more context to the full site scanning not working that @christophermhinds mentioned.

Free: 1.10.2
Pro: 1.6.0
Full site scan works as expected

Free: 1.11.0-alpha.1
Pro: 1.6.0
Full site scan pass two doesn't run
No console or log errors

Free: 1.11.0-alpha.1
Pro: Version 1.7.0-alpha.1
Full site scan pass two doesn't run
No console errors
It does log a deprecation. Should it be doing if both versions are the current alpha? Maybe you need to check for the edac_summary for backward compatibility?
[18-Apr-2024 18:02:19 UTC] PHP Deprecated: Function edac_summary is deprecated since version 1.9.0! Use EDAC\Inc\Summary_Generator instead. in /Users/stevejones/Local Sites/edactest/app/public/wp-includes/functions.php on line 6078

Free: 1.10.2
Pro: 1.7.0-alpha.1
Full site scan works as expected
Same deprecation log.
[18-Apr-2024 18:02:19 UTC] PHP Deprecated: Function edac_summary is deprecated since version 1.9.0! Use EDAC\Inc\Summary_Generator instead. in /Users/stevejones/Local Sites/edactest/app/public/wp-includes/functions.php on line 6078

These findings lead me to believe the issue is in Free: 1.11.0-alpha.1

Copy link
Member

@SteveJonesDev SteveJonesDev left a comment

Choose a reason for hiding this comment

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

_edac_issue_density and _edac_post_checked_js aren't being migrated into _edac_summary. Maybe that's for a reason?

Also, _edac_issue_density does exist in _edac_summary as issue_density but the values don't match.

Before:
Screenshot 2024-04-18 at 2 49 57 PM

After:
Screenshot 2024-04-18 at 2 50 53 PM

Full site scan not working detailed here: #578 (comment)

Add a page to the Wiki detailing the proper usage of meta in the plugin.

@pattonwebz
Copy link
Member Author

pattonwebz commented Apr 18, 2024

@SteveJonesDev I found the issue with the JS scan not working but it isn't proving easy to find a solution that doesn't involve rebuilding much of the JS scan checking logic in pro. It uses some direct queries against meta keys to count posts and more direct queries to update the values.

It probably means that the _edac_post_checked_js meta is another key that will need to live outside of the single meta - unless a lot of attention is given to the JS scan logic in pro to refactor avoiding the need for the dedicated meta key. I'll push up the changes after I have done more local testing on it.

I know the pro plugin's refactoring is coming up, so I propose refactoring that logic as part of the overall plugin refactoring work. I can make a card on the pro repo.

@SteveJonesDev SteveJonesDev marked this pull request as draft April 22, 2024 17:48
@pattonwebz
Copy link
Member Author

After some discussion and undertaking the work required to reduce post meta, it was decided to postpone this. The reduction in post meta rows was minor, but the changes were far-reaching (14 files changed in this PR and the value gained is small. These changes don't entirely account for all the post meta used by the pro plugin, which is an even more significant chunk.

After reviewing the work, it is clear that to get the most value from the changes, it requires redesigning how the suite of plugins handles data entirely, particularly column filtering and how the full site scan—the JS portion especially—deals with their status and tracking. This would be a breaking change requiring a major version bump and not something that can be squeezed into a typical release.

@pattonwebz
Copy link
Member Author

Closing this PR now, it is diverged too much to be useful and in the end I don't think we can tackle this in this way anymore. It would need a ground up rethink of data handling rather than a top down refactor.

@pattonwebz pattonwebz closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use fewer postmeta values
2 participants