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.

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

Building on multiple PHP versions, and remove auth token #120

Closed
wants to merge 57 commits into from
Closed

Building on multiple PHP versions, and remove auth token #120

wants to merge 57 commits into from

Conversation

XedinUnknown
Copy link
Contributor

@XedinUnknown XedinUnknown commented Feb 2, 2021 •
edited
Loading

  • Will solve problems described in Update dhii\module-interface #100.
  • Now requiring PHP 7.1.
  • Build on PHP 7.1 through 8.0
  • Now using PSR-4.
  • Add lockfile for consistency between dep installations.
  • Now using a distributed module system with Composer Merge Plugin.
  • Now using modularity standard version 0.3.
  • Configured PHPCS.
  • Cleaned unused deps and other things.
  • Add Psalm (allowed to fail for now).
  • Remove auth token.
  • Bring workflow in line with the rest of Inpsyde plugins.

@XedinUnknown XedinUnknown marked this pull request as draft February 2, 2021 17:04
@XedinUnknown
Add Docker, correct dep constraints
bb491ac 
It's not working because it uses deps which are incompatible with the project spec. Composer deps need to be installed, and the lockfile committed, but they cannot be installed because this project is not compatible. Bump to PHP 7.1 is strongly recommended.
@XedinUnknown
Copy link
Contributor Author

Due to a series of factors, I stronly recommend using PHP 7.1 for this project. Not only will this fix many existing incompatibility issues, but will also make the project significantly more maintainable, and compatible with a wider range of libraries and tools that make maintainability much cheaper.

@XedinUnknown
Copy link
Contributor Author

XedinUnknown commented Feb 2, 2021 •
edited
Loading

Right now, the build is failing. This is because there isn't a Composer lockfile, so it tries to update on its own - which is not something that should happen in a plugin. And I cannot add the lockfile, because I cannot install deps: the project of PHP 7.0 (this was not previously declared) depends on PHP 7.1 libs.

@XedinUnknown
Copy link
Contributor Author

Nevertheless, this PR brings things like Docker to the mix, which will make installing deps on the correct version of PHP much easier. I will add Gulp instructions too, when the lib compatibility problem is resolved. For overall picture of improvements, see wp-oop/plugin-boilerplate.

@XedinUnknown
Copy link
Contributor Author

One important question - maybe for @websupporter - is: why not use PSR-4? Is the WP class file naming convention a hard requirement of this project?

@XedinUnknown XedinUnknown self-assigned this Feb 2, 2021
@jorgeatorres
Copy link
Member

Hi @XedinUnknown!

Thanks for the PR. A couple of comments:

  • While not overly critical, this is a WooCommerce (and thus WordPress) plugin so we definitely want it to adhere to the WordPress standards as much as possible. Unless there's a valid reason not to do that.
  • Regarding the PHP 7.0 vs PHP 7.1 issue, this is something we're discussing internally. WooCommerce core recommends 7.3 but the requirement is 7.0. PayPal Payments should work wherever WC core does, but if you could elaborate on some of the reasons for possibly requiring 7.1, that could help steer the decision.
  • Some of the changes you're bringing into the plugin look great (Docker, etc.) and definitely help with testing, etc. Only thing worth mentioning is that the GitHub actions aren't supported by this repo right now, so the Travis integration shouldn't be removed.

@XedinUnknown
Copy link
Contributor Author

Thanks for the quick response, and sorr for the delay! Here are my notes to your comments:

  • We understand that you want to adhere to the WordPress standards and of course we will do so too. The reason why I suggested PSR-4 and why I would prefer it is because PSR-4 is technically better for autoloading and would be more convenient for development. If the current classmap approach is important, I am happy to leave it as is.
  • I know that there is already an on-going discussion about the minimal PHP requirement for this project. Adding to any points that may have already been mentioned, in my opinion it could be a good idea to upgrade to PHP 7.1 because many of the newer tools, standards, and libraries that could greatly reduce the amount of time spent on development, debugging, and code reviews require at least PHP 7.1. One of them is Psalm, and it would allow us to automatically verify the correctness of the code in many cases. The same can be said about interop standards, and other libraries that would remove the need for duplicate effort.
  • As far as I know, Github Actions are automatically supported in every Github repo - free for any public project, including this one. I am aware that for this repo Actions may have been disabled. As we are working with Github Actions in all of our other plugins, we would prefer that. I understand if you would like to keep using Travis. Thus my suggestion is to keep using Travis for what it is already used for (PHPCS checks), and do the rest in Github Actions. Would that work for you?

@jorgeatorres
Copy link
Member

jorgeatorres commented Feb 5, 2021 •
edited
Loading

Hi @XedinUnknown!

[...] because PSR-4 is technically better for autoloading and would be more convenient for development. If the current classmap approach is important, I am happy to leave it as is.

I don't feel strongly about this and we can always exclude some of the rules from the WP standard if the benefit is worth it. Could you give some examples of how some classnames/directories would have to change?

Adding to any points that may have already been mentioned, in my opinion it could be a good idea to upgrade to PHP 7.1 [...]

Got it. We've only found 7 extensions on the marketplace that explicitly require PHP > 7.0. While this is something we're taking into account, it's not necessarily the only way to look at this matter. We're trying to obtain some stats from current users to decide.
Quick question: if we bump the requirement to 7.1, would this allow building the extension under PHP 8.x? Right now, dependencies prevent this (Composer can't resolve everything successfully).

As far as I know, Github Actions are automatically supported in every Github repo - free for any public project, including this one. I am aware that for this repo Actions may have been disabled.

We were seeing some errors with actions in this repo due to the repo being in a legacy plan, but it seems those were from the time the repo was private. I did a quick test and it is true that actions are available now that it is public.
As long as we have a workflow for running unit tests and PHPCS, feel free to make the changes, including removing the Travis integration.

Thanks for working on this!

@XedinUnknown
Install WC into env
d0604b0 
Can now control WP and WC versions.
@XedinUnknown
Build in PHP min requirement
ec77951
@XedinUnknown
Bump min PHP version and include lockfile
71abc53 
The new version requirement will allow newer versions of dependencies to be used, which will increase compatibility with other products.

The lockfile ensures that the same deps will be installed everywhere, be it on another dev's machine, or on the CI server. This way we can actually test with the deps that are going to be shipped, instead of a random set.
@XedinUnknown
Now depending on newer module standard
82e57e8 
Refactoring of modules themselves was not necessary,
because they were already following the new standard.
@XedinUnknown
Use Composer modules and convert modules to PSR-4
6bc2d0b 
PSR-4 is much more robust and predictable. But to do this,
a source root dir must be specified for every module.
This could be done in the root file, but this is not very modular.
Instead, now every module declares its own source root by using
the amazing Composer Merge Plugin. This approach allows each module
to also declare its own dependencies. Together, these changes allow
modules to be easily extractable to separate pacakges when the need
arises, and in general improves modularity significantly.
@XedinUnknown
Remove unused dependency
50a4905 
The WP containers don't seem to be used anywhere. There were
imports, but they are not actually used it seems.
@XedinUnknown
No longer requiring WPCS for file names
6633496
@XedinUnknown
Updated PHP test version for PHPCS
1bf959c
@XedinUnknown
Configure PHPCS
6fdfdbd 
Now will use config instead of CLI values for most things.
Will also show sniff codes.
@XedinUnknown
Add Container typehint to all service definitions
00ce80b
@XedinUnknown
Build on PHP 8, and not on PHP 7.0
3bfc3de
@XedinUnknown
Try update PHPUnit before installing packages
732cbb8 
This could go around the fact that no single PHPUnit
major version supports PHP 7.1 and 8.0 at the same time.
@XedinUnknown
Optimize dep installation on CI
9fc3f00
@XedinUnknown
Now using modularity standard v0.3
e4a93dd 
This is the newest release, and is the first one that is compatible
with PHP 8. Other changes are BC-breaking, but very very minor.
@XedinUnknown
Upgrade PHPSpec on CI
3d7a82f
@XedinUnknown
Copy link
Contributor Author

This builds for me in XedinUnknown#1.

@XedinUnknown XedinUnknown marked this pull request as ready for review June 7, 2021 00:35
@XedinUnknown
Copy link
Contributor Author

@Dinamiko, after synchronizing with upstream, build is now failing. Would you be able to take a look, please?

This was referenced Aug 19, 2021
Merged
Merged
@XedinUnknown XedinUnknown changed the base branch from trunk to docker-fixes August 25, 2021 16:34
@XedinUnknown XedinUnknown changed the base branch from docker-fixes to trunk August 25, 2021 16:35
@XedinUnknown XedinUnknown changed the base branch from trunk to docker-fixes August 25, 2021 16:37
@XedinUnknown XedinUnknown changed the base branch from docker-fixes to trunk August 25, 2021 16:37
This was referenced Aug 26, 2021
Merged
Closed
Merged
@Dinamiko
Copy link
Contributor

Already included after 1.6.1 release.

@Dinamiko Dinamiko closed this Nov 24, 2021
@lokacoding lokacoding mentioned this pull request Nov 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgeatorres jorgeatorres Awaiting requested review from jorgeatorres

Assignees

@Dinamiko Dinamiko

@XedinUnknown XedinUnknown

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@XedinUnknown @jorgeatorres @Dinamiko @strangerkir