search

prismicio-community / php-kit

Community maintained development kit for Prismic and the PHP language
https://prismic.io
Other
108 stars 82 forks source link

feat!: support PHP 8 #190

Closed c0nst4ntin closed 1 year ago

c0nst4ntin commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

angeloashmore commented 2 years ago

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 commented 2 years ago

@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 commented 1 year ago

@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: https://github.com/prismicio-community/php-kit/issues/182

lihbr commented 1 year ago

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:

    • 190, this PR, by @c0nst4ntin

    • 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.

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

c0nst4ntin commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@JeroenBoersma I'll gladly join this call as well 👍🏼

I figure it's only fair to give you a heads up on what we talked about. Currently the plan is to move forward with this PR and then optimize some of the code in minor releases afterwards. As Prismic themselves currently don't have the resources to fully work together with the PHP Community in the way they would love to, we agreed that going forward I will assist @lihbr and @angeloashmore in working on this package.

This means that once a workflow is established and some decisions to the contribution guidelines were made we hope to bring this SDK back to its old glory and will start working on the current issues and PRs. Some things that I would love to improve over time is the code quality of the package and creating a better contribution workflow.

I would love to hear your opinion on these changes and what you would like to see coming to this SDK in the future. If you like we two could set up a small meeting and discuss about everything regarding PHP (just send me an E-Mail to contact@constantinross.com). I'd be happy to meet you 😄

angeloashmore commented 1 year ago

Hey @c0nst4ntin, I've just added you to the repository as a maintainer! 🎉

I'll let you merge this PR in case there're any final changes you'd like to make first.

Once it's merged, you can make a new Git tag to publish a new version. As you stated in your original message, we should make this a new major version: v6.0.0.

If you have any questions about moving forward, please don't hesitate to reach out. 🙂 You can DM me on Twitter if you have any immediate questions: @angeloashmore

Thanks again!

c0nst4ntin commented 1 year ago

Thank you @angeloashmore! Later I will do some final checks and then prepare everything for the new version 6.0.0

If I have any further questions or run into any problems I will reach out to you via Twitter.

c0nst4ntin commented 1 year ago

@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

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

c0nst4ntin commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

c0nst4ntin commented 1 year ago

@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 commented 1 year ago

@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:

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 commented 1 year ago

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. 🙂