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

Cleaning up codebase (WIP) #722

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

nmolham-godaddy
Copy link
Contributor

Summary

Cleaning up and optimize the codebase.

Details

There are so many aspects the codebase can be cleaned, so I thought about opening a PR to get feedback and inputs.

QA

  • Code review
  • Checks pass

Before merge

  • I have confirmed these changes in each supported minor WooCommerce version

Copy link
Contributor

@agibson-godaddy agibson-godaddy left a comment

Choose a reason for hiding this comment

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

I'm fine with these changes in theory, but I'm just "requesting changes" here because I think this can only go into a major release.

These updates are to abstract classes, which we extend and override in plugins. Just doing a normal composer update would immediately break those classes, e.g.

image

All the overridden methods (and properties) would have mismatching signatures, which will require manual work to update.

So I'm okay with it, but I think it has to be put in a major release like v6.0.0.

@nmolham-godaddy
Copy link
Contributor Author

@agibson-godaddy Agreed, these changes are are considering to be breaking changes. I was also thinking about raising the PHP min version to PHP 8.0, but I realised that would be unrealistic considering WooCommerce requirements.

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.

2 participants