prismicio-community / php-kit

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

PHP 8.1 support #182

Closed peterjaap closed 2 years ago

peterjaap commented 2 years ago

On PHP 8.1, I'm getting this 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

Afaik there's nothing stopping us from changing the constructor to;

diff --git src/Prismic/Form.php src/Prismic/Form.php
index 7de168d..5f5c027 100644
--- src/Prismic/Form.php
+++ src/Prismic/Form.php
@@ -63,12 +63,12 @@ class Form
      * @param array  $fields    the list of Prismic::FieldForm objects that can be used
      */
     private function __construct(
-        ?string $name = null,
-        string  $method,
-        ?string $rel = null,
-        string  $enctype,
-        string  $action,
-        array   $fields
+        string $name = null,
+        string $method,
+        string $rel = null,
+        string $enctype,
+        string $action,
+        array  $fields
     ) {
         $this->name    = $name;
         $this->method  = $method;
peterjaap commented 2 years ago

Here's another one;

Exception #0 (Exception): Deprecated Functionality: htmlentities(): Passing null to parameter #2 ($flags) of type int is deprecated in /data/client/magento2/vendor/prismic/php-sdk/src/Prismic/Dom/RichText.php on line 154

c0nst4ntin commented 2 years ago

@peterjaap I ran into similar problems on some client projects and therefore looked into this topic a little bit more. I created a new pull-request (#190) that contains similar changes to #183 but also solves other challenges such as making PHP 8 a minimum requirement and updating the test suite to work with the new version of PHP.

peterjaap commented 2 years ago

Looks like Prismic has completely forgotten or neglected PHP. Maybe @srenault can help us out by getting Prismic to give the PHP community some love?

c0nst4ntin commented 2 years ago

@peterjaap I was in contact with some of the developers at Prismic previously. I'll try to address this and see if we can get this repository back to its old glory. Maybe even improve some of the formatting and code quality

pTinosq commented 2 years ago

Any updates on this? Really praying for 8.1 support soon ๐Ÿ˜ฅ

peterjaap commented 2 years ago

@pTinosq we've given up on buy-in from Prismic. They're a JS crowd, they don't like PHP. You can use our fork instead https://github.com/elgentos/prismicio-php-kit

pTinosq commented 2 years ago

@pTinosq we've given up on buy-in from Prismic. They're a JS crowd, they don't like PHP. You can use our fork instead https://github.com/elgentos/prismicio-php-kit

Oh wow that's really annoying. Should I use a specific fork or is the elegentos version sufficient?

peterjaap commented 2 years ago

@pTinosq the elgentos one is most likely sufficient if you're looking PHP 8.1 compat

c0nst4ntin commented 2 years ago

@pTinosq They are currently taking a closer look at this PR: https://github.com/prismicio/php-kit/pull/190

If I followed along on Twitter correctly @angeloashmore was recently visiting the office and is now on holiday. Let's wait and see what comes from the PR. I trust, that they will give us an update shortly.

As they mentioned in the PR of mine they are simply focussing more on JS but still care about PHP:

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

The PR fully supports PHP 8 and passes all CI Checks implemented on this repository as well.

pTinosq commented 2 years ago

Ah that's good. I'll keep my notifications on for the PR. Hopefully Prismic can pull through ๐Ÿ™๐Ÿ™

c0nst4ntin commented 2 years ago

@peterjaap @pTinosq I'm happy to tell you, that this issue was fixed in feat!: support PHP 8 #190

To make use of this change please upgrade to Version 5.2.0

I will proceed to close this issue.

peterjaap commented 2 years ago

@c0nst4ntin thanks!! ๐Ÿ™Œ

pTinosq commented 2 years ago

@c0nst4ntin This is the news I needed to wake up to. Fantastic stuff ๐ŸŽ‰๐ŸŽ‰