feat!: support PHP 8

[c0nst4ntin]

As mentioned in Issue #182 this package currently does not support PHP 8 (and PHP 8.1).
There are multiple problems that I tried to solve with this PR:

1. Required Parameters must be declared before optional Parameters

As previously mentioned in #182 the Form class has 2 optional parameters in the constructor followed by several required parameters. This causes the following Notice:

Deprecated: Optional parameter $name declared before required parameter $fields is implicitly treated as a required parameter in /data/client/magento2/vendor/prismic/php-sdk/src/Prismic/Form.php on line 65

To fix this issue I removed the default value for the parameters but left them nullable. The Code inside the package is not affected by this change, as it already passes a fallback null.

2. The second Parameter of htmlentities() is no longer nullable (PHP Docs)

Although this was already partially solved by the current open Pull-Request #183 this PR delivers complete support for PHP 8 instead of just solving this one Exception. I do not want to claim credit for this change, but it is required to make this PR work as a whole migration to PHP8.

3. Updating the dependencies and tests to support PHP 8

To support Testing in PHP 8 I updated the dependencies to use PHPUnit Version 9 (Version Support). With this came multiple changes to test files, as certain Functions were deprecated or removed in the new versions of PHPUnit.

To allow all tests to continue working I had to install the packages phpspec/prophecy and phpspec/prophecy-phpunitas a dev-dependency.

Furthermore, I made PHP 8.0 a minimum requirement in the packages composer.json file.

Obviously, this PR introduces some breaking changes and should therefore be considered as part of a new major version.

[peterjaap]

@c0nst4ntin good work! Unfortunately it looks like Prismic does not care about PHP anymore and has abandoned this repo. We’ve made our own fork and continued working there -> https://github.com/elgentos/prismicio-php-kit

[c0nst4ntin]

@c0nst4ntin good work! Unfortunately it looks like Prismic does not care about PHP anymore and has abandoned this repo. We’ve made our own fork and continued working there -> https://github.com/elgentos/prismicio-php-kit

@peterjaap Thank you! The changes you made to your fork look somewhat close to this PR although I also got the phpDocumentor working again with the Travis CI. I reckon they definitely care about PHP, but shifted their main focus towards frontend-oriented Frameworks etc.

@lihbr I know it's been some time since you, me and @NouhaC talked about Prismic but could you please make sure that this PR gets to the right person? Without these changes, the Repository and Prismic as a CMS could become obsolete for future PHP Developers. We would need to create our own Forks and work individually instead of as a community.

I'll gladly help to maintain parts of this repository as much as I can.

[lihbr]

Duly noted @c0nst4ntin , just pinging @angeloashmore also, we'll get back to you soon!

[angeloashmore]

Thanks for the ping, @lihbr, and awesome work, @c0nst4ntin!

It's true that our efforts are focused on JavaScript these days, so we would love to work together on keeping the PHP library up to date. @lihbr and I will discuss how we can arrange some form of collaboration.

In the meantime, this PR isn't being ignored! Before merging, however, we'll need to do some testing of our own.

@peterjaap If you have any comments/feedback on the PR as well, we're more than happy to listen. 🙂

[c0nst4ntin]

@angeloashmore Great. In the meantime let me know if you need anything! Once this package is ready for PHP 8 I might look into migrating some of the other starter repositories for PHP (e.g. php-website)

[c0nst4ntin]

@angeloashmore Hey There 👋🏼 I was just wondering how things are going with your testing? There was a recent inquiry from another developer as to the state of this PR on the underlying Issue: #182

[lihbr]

Hey there, first of all, @angeloashmore and I are really sorry for the delay we're taking getting back to you.

We'd like to organize our process better to ensure the PHP SDK remains maintained and usable by PHP users while providing them with the support they need.

1. The first action we took towards that goal is moving this repository from our company organization (@prismicio) to our community organization (@prismicio-community) to better reflect the maintenance happening on this kit. We also updated the pointer on Packagist.

2. There are currently two PRs that update the kit to support PHP 8:

   • feat!: support PHP 8 #190, this PR, by @c0nst4ntin
   • Upstream Merge request - we created a fork because we thought the project was dead 😺  #191, by @JeroenBoersma

   Our goal is aligned with yours and we'd also like to support PHP 8 with the kit. We'd value having you onboard throughout that journey.

3. We'd like to meet you, @c0nst4ntin, @JeroenBoersma, and anyone else interested in our PHP kit, so we can coordinate more quickly on making this kit support PHP 8.

   • Would you be up for a call about it? If yes, please fill in this poll with your email so we can schedule something between all of us :)
   • If a call doesn't work for you, we're still really interested in hearing from you and establishing a more direct line of communication, please mail me at: lucie.haberer@prismic.io

Thank you for your understanding, we're looking forward to seeing the PHP kit get back on track~

[c0nst4ntin]

@lihbr Thank you for this opportunity. I filled in my information so we can get on a call as soon as possible.

Personally, I feel like the PR from @JeroenBoersma and @peterjaap does more than just a PHP update. This obviously comes from the reasoning behind their fork, which was to continue the development of their organisation.

Whilst some of the changes look promising, I believe we should go forward with this PR implementation and revisit some of their changes after the support of PHP 8 was added. This was we can benefit from all our insights

[gsteel]

@c0nst4ntin - you may also be interested in https://github.com/netglue/prismic-client and https://github.com/netglue/prismic-cli - these support up to 8.1 right now but 8.2 support is forthcoming.

[c0nst4ntin]

@gsteel Thank you for the resources. I'll look into them 😄 But let's also try to get the official package to support PHP 8 so projects can continue to use it (for eg. when being migrated)

[gsteel]

@gsteel Thank you for the resources. I'll look into them 😄 But let's also try to get the official package to support PHP 8 so projects can continue to use it (for eg. when being migrated)

👍 - Sure thing. I personally abandoned this lib a long time ago. Some of the work in the above may be useful to you in future is all. Good luck with it :)

[JeroenBoersma]

Filled out the form, didn't hear back, was out for a annual leave...

Personally, I feel like the PR from @JeroenBoersma and @peterjaap does more than just a PHP update. This obviously comes from the reasoning behind their fork, which was to continue the development of their organisation.

I get it that it feels this way, we indeed touched more lines of code. Thought, nothing public API specific besides of PHP 8.1 support and we've splitted the RichtText Block the same way as JavaScript does it. This makes extensibility, testing and maintainability easier long term. That's why we've added it... Maybe we can even rebase on this one and make it a collaboration =)

[c0nst4ntin]

Hey @JeroenBoersma 👋 Yesterday I spoke to some of the developers at Prismic. Sad to hear, that you couldn't make it. We figured to move forward with #190 and then revisit some of your refactorings after PHP 8 was supported. Does that sound okay to you? Some of your changes look promising, so maybe we could add them as a clean PR after the update 😊

[JeroenBoersma]

Let's do that and get this project moving... :D - would love to have one source of truth. Would love to catch up with @lihbr and the dev team to see what's in store for the upcoming time!

[c0nst4ntin]

@lihbr @angeloashmore I fixed some of the links in this repository to account for the change in organizations. I believe there is a problem with the Travis CI 🤔

Currently, there are no new builds coming from my changes: https://app.travis-ci.com/github/prismicio/php-kit/requests

• https://app.travis-ci.com/github/prismicio
• https://app.travis-ci.com/github/prismicio-community

Maybe you could take a look at this and make sure we can get this running again?

[c0nst4ntin]

@gsteel Do you see anything else that should be checked or changed before releasing the new major version?

Otherwise, I would wait until Prismic updates the Travis configuration and then merge this PR.
I might also merge #189 into the master before creating the new version.

[gsteel]

IMO, considering you're going for a major, I'd wait and see what else can be improved that also breaks BC.

That said, AFAICT, nothing in this patch actually breaks BC. SemVer allows dependency changes in a minor (Composer has also got your back there). If it were up to me, I'd release this as a minor as soon as CI passes.

WRT CI, I'd also advocate GHA over travis. The Prismic client I maintain uses a pretty good setup courtesy of the @laminas org: GHA Yaml, Example Run

[c0nst4ntin]

@gsteel Thank you for your input on this. I really value your opinion, as you have been actively contributing to this repository and community for a longer time. I got one final question before I think this PR is "final" 😄

I also found [Bug] APC and APCU mismatch which were attempted to be solved in Fixes #162 - Correctly checks for apcu #163

I can confirm that extension_loaded('apc') returns false and extension_loaded('apcu') returns true for my local setup as well. Sadly the repository behind this change was deleted, so it can't be reopened.

This change was also made in this closed PR PHP 8.0 support & patches #173

Do you think I should add this to this PR? Seems to be legit, right?

[gsteel]

From what I remember, this lib has a hard dependency on apcu - this should be added to require as "ext-apcu": "*" - at this point extension_loaded is irrelevant and can be removed - however, if it's actually a soft or dev dependency - apcu should still be required in dev, or mentioned in suggest and the extension_loaded should refer to apcu - apc is long dead and not available for PHP 8+ (I don't think), so no BC break either.

[c0nst4ntin]

@gsteel I implemented the version which I think is best. Once this is merged I will close the other Issue and mention this.

I would still tend to go for a new major version. I think, updating the required PHP version and fixing this apcu check could maybe lead to some unwanted behaviour if it gets released as a minor.

Anything else you would like to check or add? Otherwise, this now only waits for the CI to get fixed and then this can be released.

Once again, I am happy to have you on board

[gsteel]

Hey @c0nst4ntin I disagree with the decision on the major: https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api - The public API hasn't changed (Removal of the null default params is debatable), and the APCu alteration is satisfied by the dependencies - it's essentially a bug fix - apc was last released over 10 years ago. This lib is very outdated and I'm confident that you'll shortly want to break BC and be forced to release 7 - it's only opinion though so 🤷‍♂️

[c0nst4ntin]

Hey @c0nst4ntin I disagree with the decision on the major: https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api - The public API hasn't changed (Removal of the null default params is debatable), and the APCu alteration is satisfied by the dependencies - it's essentially a bug fix - apc was last released over 10 years ago. This lib is very outdated and I'm confident that you'll shortly want to break BC and be forced to release 7 - it's only opinion though so 🤷‍♂️

So you would suggest releasing the changes as version 5.2.0 instead of a new major? I can see your point of view.

Is the change to htmlentities() definitely backwards compatible (Docs)?

[gsteel]

Yes, the changes to htmlentities preserve BC because null and the default flags are equivalent. Obviously, you have to provide the flags because the encoding has always been explicitly declared as utf-8. IMO 5.2.0 is fine.

[c0nst4ntin]

@angeloashmore @lihbr Then I would also go for version 5.2.0 or do you have any reason to go for a major version?

This PR now only waits for Prismic to solve the CI problems and reply to this message 🙂

Thank you @gsteel for the review and insightful thoughts 💪🏼

[angeloashmore]

@c0nst4ntin Publishing a minor works for me as long as existing projects won't break by upgrading. We can reserve v6.0.0 for a more significant overhaul if that's what you'd like to do.

We tend to be conservative when it comes to publishing breaking changes, ideally not more than once a year. Definitely not a hard rule, just a way to ensure we're giving developers enough time to update and maintain their projects.

Re: Travis CI — I don't have access to fix the Travis CI issue, but I reached out the team that does. I'll let you know once I hear back. 🙂

[angeloashmore]

@c0nst4ntin We use GitHub Actions for our in-house maintained packages since it's simpler to manage. Feel free to migrate to GHA if you'd like. 🙂 I'm still checking how we can get Travis CI running again though.

[c0nst4ntin]

@angeloashmore Once this PR is released and we established a more direct way of communication, we can discuss further changes to contribution guidelines, refactorings and migrating to GitHub Actions.

I just dropped the current state of this PR into an old production Website of mine and everything continued to work. So there should be no major problems as far as @gsteel and myself are aware.

We can then keep the version 6.0.0 for a bigger API facing change, if we are interested in perusing this direction.

Once Travis is back on track, just give me a short heads up including the link. Do you mind releasing a new version on a Friday or should we wait until Monday?

[angeloashmore]

@c0nst4ntin Releasing on a Friday shouldn't be a problem. Packages are versioned, so if there's an issue in the package or the publishing flow, developers can always stick to the previous version. Whatever works best for you is good. 👍

I'll let you know once I hear back about Travis CI. It may not be this week, FYI.

[angeloashmore]

@c0nst4ntin So it looks like Travis CI is working correctly except for this PR. I just tested it in PR #193, and we can also see @gsteel's PR #192 also triggered Travis CI. I'm not sure why this specific PR got disconnected.

Might be because we moved the repo after it was opened.

As long as tests are passing on your machine, I think it's safe to move forward and merge. If you want to be cautious, you can close this PR and re-open a new PR, which should reconnect to Travis CI.

(Or alternatively, change to GitHub Actions. Totally up to you!)

[gsteel]

Actions are currently disable for this repo I believe @angeloashmore - could you activate them?

[c0nst4ntin]

@angeloashmore I think you might be right. That's also why the two other PRs worked. Does the merge also trigger a CI Check for the master? In that case, I think we can just merge? 🤔

If we close this and re-open we would lose all the communication in context with the changes.

[c0nst4ntin]

@angeloashmore I merged the PR, which resulted in a new Travis Build. This sadly failed: https://app.travis-ci.com/github/prismicio-community/php-kit/builds/257470938

As far as I can tell, all checks passed:

• The command "composer test" exited with 0
• The command "composer cs-check" exited with 0
• The command "composer docs" exited with 0

Whilst there are a few warnings regarding the APC extension, it appears that the last step failed to publish the docs:

Preparing deploy
failed to deploy

This is also, why your PR did not fail:

Skipping a deployment with the pages provider because the current build is a pull request.

Whilst I don't think, that this indicates a problem with releasing the new version, I would like to hear your feedback. I'd guess this is up to your "Travis Team" to figure out?

Should I go forward and create a new tag and release? Or wait until we are able to re-run the failed steps?

[angeloashmore]

Hey @c0nst4ntin, looking at the build logs, it looks like CI failed to push the PHPDoc build to GitHub pages. The GitHub token configured in Travis was invalid since moving the repository to prismicio-community.

It's fixed now with a new token, so you should be able to create new commits and PRs with Travis CI. If you have time, could you test it on your side and let me know if it works?

You can go ahead create a new tag and release the updates whenever you're ready. 🙂