Drop PHP 7, add PHP 8.2 Support #39

rogervila · 2023-01-10T17:13:26Z

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	no
RFC	no
QA	no

Description

Drop PHP 7, add PHP 8.2 Support

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila · 2023-01-11T08:55:36Z

Hello @weierophinney @Ocramius,

This PR is part of the Laminas API Tools PHP 8.2 support that I am working on.

Most of the modifications are Psalm and PHPCS error fixes. Let me know if I should apply any other changes.

Thank you.

.gitignore

Signed-off-by: Roger Vilà <rogervila@me.com>

gsteel

Thanks for the huge effort @rogervila - This is just a general skim - I don't have a deep knowledge of this project so I won't be able to ultimately approve this patch without review from other team members 👍

src/Collection.php

src/EntityHydratorManager.php

src/Link/Link.php

src/Metadata/Metadata.php

src/View/HalJsonStrategy.php

test/Plugin/HalTest.php

test/Plugin/TestAsset/JsonSerializableEntity.php

test/TestAsset/JsonSerializable.php

weierophinney · 2023-01-11T19:10:18Z

I'll review after you've incorporated the feedback from @gsteel .

Thanks, @rogervila !

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila · 2023-01-12T10:47:38Z

@weierophinney @gsteel I have applied the required changes. Let me know if there is something else to change.

gsteel

Thanks for incorporating the feedback @rogervila, a couple of notes, but I'll hand over to @weierophinney now :)

gsteel · 2023-01-12T11:00:52Z

src/Link/Link.php

-     *      @var string|array $route Optional.
-     *      @var string $url {@deprecated since 1.5.0; use 'href' instead} Optional.
-     * }
+     * @psalm-param array{


FYI, you can add a psalm type to the class-level doc block such as:

/** * @psalm-type LinkArraySpec = array{ * rel: string|whatever * other: list<whatever> * } */

And then use that type for your parameter and return docs: @return LinkArraySpec instead of copying and pasting the entire spec around.

In other classes, the type can be imported with @psalm-import-type LinkArraySpec from ImportedClassName on the class-level doc block.

Hi @gsteel, I have tried to extract it to a type, but then psalm raises some MixedArgumentTypeCoercion errors due to the fact that the initial status of $links property at src/Metadata/Metadata.php:88 is initialized as an empty array (read as array<empty,empty> by psalm)

Any change on the initial property status would be considered a BC, so I suggest keeping the duplicated $spec Psalm definition as added on the latest commit.

What do you think?

You can make each of the keys optional:

/** * @psalm-type LinkArraySpec = array{ * rel?: string|whatever, * other?: list<whatever> * } */

Doing so gives you both the correct shape AND allows for empty arrays.

gsteel · 2023-01-12T11:12:25Z

phpcs.xml.dist

        <exclude-pattern>src/Plugin/Hal.php</exclude-pattern>
        <exclude-pattern>test/ResourceFactoryTest.php</exclude-pattern>


These exclusions shouldn't be necessary - The factories all reference Interop\ContainerInterface which is probably class_aliased causing the false negative for phpcs, so that's fine, but if an CS error is showing up for these two, it's likely an actual error (?)

Thank you @gsteel

By removing them, I get the following error:

Expected class name interop\container\containerinterface; found Interop\Container\ContainerInterface Expected class name interop\container\exception\containerexception; found Interop\Container\Exception\ContainerException

I am not sure why PHPCS requires the class to be lowercase. Is there any other solution instead of ignoring those files?

I am not sure why PHPCS requires the class to be lowercase.

This is an error and the class name should be reverted.

Yes @froschdesign, I did not set the class name to lowercase. Instead, I ignored the error on the PHPCS config file.

So, if you agree, I will keep the ignored errors on the PHPCS config file since we do not want to set those classes to lowercase. Is that ok @froschdesign, @gsteel?

Yes - specifically I was talking about the suppressions for Laminas class names - theses should not be suppressed. The class names from Interop should be correctly cased in the code i.e. Interop\Container\ContainerInterface and then the invalid error surfaced by PHPCS should be suppressed. The error itself stems from class_alias stuff from laminas-servicemanager and a possible bug in the related sniff - In theory, if you bump laminas-servicemanager to the correct minimum version (^3.11, but preferably ^3.18) where it replaces interop, you can drop Container Interop from deps completely, replace everything with Psr\Container and remove all the suppressions in phpcs.xml - but it's probably best to keep that for another patch as I'm not sure how well that will play with other deps in this project.

Thank you. I will apply this change on the next commit.

weierophinney

Looking good!

Themes during my review:

We shouldn't be excluding entire classes from CS checks. Use // phpcs:disable <check name> / // phpcs:enable <check name> blocks instead (or disable specific rules at the file level).
In factories, prefer assertions of returned types pulled from the container to using @var annotations.

I've also noted a place where we're using call_user_func_array and proposed an alternative implementation.

weierophinney · 2023-01-12T14:21:27Z

src/Factory/HalJsonRendererFactory.php

+        /** @var HelperPluginManager $helpers */
+        $helpers = $container->get('ViewHelperManager');
+        /** @var ApiProblemRenderer $apiProblemRenderer */
        $apiProblemRenderer = $container->get(ApiProblemRenderer::class);


Since this is a factory, it might be better for us to perform assertions on the returned types instead of using annotations:

$helpers = $container->get('ViewHelperManager'); assert($helpers instanceof ContainerInterface); $apiProblemRenderer = $container->get(ApiProblemRenderer::class); assert($apiProblemRenderer instanceof ApiProblemRenderer);

src/Factory/HalJsonStrategyFactory.php

weierophinney · 2023-01-12T14:25:13Z

src/Link/Link.php

+        $relation = $spec['rel'];
+        $link     = new static($relation);


If the array shape is defined for $spec, do we need the intermediary variable here and throughout this method?

weierophinney · 2023-01-12T14:32:55Z

src/Link/PaginationInjector.php

+        /** @var Paginator $collection */
+        $collection = $halCollection->getCollection();
+        $page       = $collection->count();


We shouldn't assert that this is a Paginator instance here. getCollection() is defined as returning an array, Traversable, or Paginator. As such, we need to check for Countable or array, and anything else we cannot know how to count.

$collection = $halCollection->getCollection(); if ($collection instanceof Countable) { $page = $collection->count(); } elseif (is_array($collection)) { $page = count($page); } else { // either throw an exception here (uncountable collection) or use a default for $page (e.g. 1) }

I will keep 1 as default to avoid BC

weierophinney · 2023-01-12T14:33:20Z

src/Link/PaginationInjector.php

+        /** @var Paginator $collection */
+        $collection = $halCollection->getCollection();
+        $pageCount  = $collection->count();


Same comment as previous.

weierophinney · 2023-01-12T14:38:14Z

src/Metadata/MetadataMap.php

+        $options           = $this->map[$class];
+        $this->map[$class] = new Metadata($class, $options, $this->getHydratorManager());


Was Psalm complaining about this? I'm not sure why we would need to have the intermediary variable here.

weierophinney · 2023-01-12T14:40:04Z

src/Plugin/Hal.php

+        /**
+         * @var mixed $value
+         */


Suggested change

/**

* @var mixed $value

*/

/** @var mixed $value */

weierophinney · 2023-01-12T14:41:13Z

src/Plugin/Hal.php

@@ -155,15 +156,16 @@ public function getController()
     *
     * @return self
     */
-    public function setEventManager(EventManagerInterface $events)
+    public function setEventManager(EventManagerInterface $eventManager)


Is the change in variable name really necessary here? Elsewhere in the class, we still use $events.

It was a PHPStorm complain. Reverting.

Is the change in variable name really necessary here? Elsewhere in the class, we still use $events.

Hi @weierophinney, Psalm is failing if I keep the $events variable name:

Error: src/Plugin/Hal.php:159:59: ParamNameMismatch: Argument 1 of Laminas\ApiTools\Hal\Plugin\Hal::setEventManager has wrong name $events, expecting $eventManager as defined by Laminas\EventManager\EventManagerAwareInterface::setEventManager (see https://psalm.dev/230)

Should I ignore or fix the issue?

Ah, okay, that makes sense - because with PHP 8, and the introduction of named arguments, a deviation in argument name when implementing an interface would be considered a potential contractual change.

Go ahead and change it, then - and note in the commit the reason why.

The problem comes from laminas/laminas-eventmanager

On the EventManagerAwareInterface, the parameter is called $eventManager while on EventManagerAwareTrait is called $events

This triggers a Psalm error no matter which variable name I use, so I have ignored it. I have also opened an Issue on the library to fix it.

I think the PR is ready to be merged. Please, @weierophinney, let me know if there is anything else to change.

weierophinney · 2023-01-12T14:42:40Z

src/Plugin/Hal.php

         */
-        $callback = function ($r) {
+        $callback = function ($r): bool {


Since we're setting the minimum PHP version to 8.0, I'd get rid of the annotation and make this simply:

$callback = function (mixed $r): bool {

src/ResourceFactory.php

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila · 2023-01-13T11:00:41Z

Hi, please check this comment so I can fix the CI

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila · 2023-01-16T11:40:15Z

Hi @weierophinney, the PR is ready to be reviewed. Let me know if any additional change is needed.

weierophinney

Themes of things I saw during this round of reviews:

Psalm suppressions were put in the psalm.xml.dist file instead of as annotations in files. They should be as close to the code as possible, otherwise we run the risk of introducing additional occurrences later that are then unflagged.
You can use phpcs:ignore to ignore a CS rule on a single line.
There's inconsistent refactoring of factories. Some are using assertions to test values returned from the container, and others are using annotations to flag to Psalm what the value is. Go with one or the other; my recommendation is using assertions, as Psalm understand them, and they give additional runtime safety.
Extract the link array spec as a Psalm type template, and use it wherever needed, instead of recreating it. Since it has some complexity to it, it would be easier to re-use than to risk having one or more go out of date.
Extract the count function (example provided for you)

psalm.xml.dist

src/Factory/HalConfigFactory.php

src/Factory/HalControllerPluginFactory.php

src/Factory/HalJsonRendererFactory.php

src/Factory/HalJsonStrategyFactory.php

src/Factory/HalViewHelperFactory.php

src/Link/PaginationInjector.php

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila · 2023-01-26T11:26:06Z

Hello @weierophinney, @gsteel, @froschdesign,

I applied the latest required changes. Let me know if there is something else to change.

jonkerw85 · 2023-04-20T11:16:44Z

What is the status? As far as I can see, this merge request is blocking the following projects from upgrading to 8.2.

rchrdkn · 2023-06-23T09:49:30Z

Hi @rogervila, @weierophinney, @gsteel, @froschdesign

What's the latest on this? It would be great to get 8.2 support for this and those blocked by it!

rogervila · 2023-06-23T10:16:39Z

Hi @rchrdkn, the PR is waiting to be approved by the core team.

Meanwhile, I suggest using this fork, which provides PHP 8.2 support.

rogervila added 2 commits January 10, 2023 18:10

Drop PHP 7, add PHP 8.2 Support

98f6a9d

Signed-off-by: Roger Vilà <rogervila@me.com>

Remove unused files

d3710d4

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila mentioned this pull request Jan 10, 2023

Drop PHP 7, Support PHP 8.2 laminas-api-tools/api-tools-content-negotiation#27

Merged

froschdesign reviewed Jan 11, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Revert additional changes

091277b

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila requested a review from froschdesign January 11, 2023 09:04

gsteel requested changes Jan 11, 2023

View reviewed changes

froschdesign removed their request for review January 11, 2023 12:20

Apply review changes

295e885

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila requested a review from gsteel January 12, 2023 10:46

gsteel reviewed Jan 12, 2023

View reviewed changes

rogervila requested a review from gsteel January 12, 2023 14:48

weierophinney requested changes Jan 12, 2023

View reviewed changes

Apply required changes

3f0074c

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila requested review from weierophinney and gsteel and removed request for gsteel and weierophinney January 12, 2023 17:21

Fix Psalm errors

cdfe74b

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila mentioned this pull request Jan 17, 2023

EventManagerAwareInterface and EventManagerAwareTrait parameters have different names laminas/laminas-eventmanager#41

Open

weierophinney requested changes Jan 17, 2023

View reviewed changes

Apply required changes

ff8c2dc

Signed-off-by: Roger Vilà <rogervila@me.com>

rogervila requested review from weierophinney and gsteel and removed request for weierophinney January 26, 2023 11:26

weierophinney added this to the 1.10.0 milestone Jul 10, 2023

weierophinney added the Enhancement New feature or request label Jul 10, 2023

weierophinney merged commit 3cc89f2 into laminas-api-tools:1.10.x Jul 10, 2023

		<exclude-pattern>src/Plugin/Hal.php</exclude-pattern>
		<exclude-pattern>test/ResourceFactoryTest.php</exclude-pattern>

		$options = $this->map[$class];
		$this->map[$class] = new Metadata($class, $options, $this->getHydratorManager());

Drop PHP 7, add PHP 8.2 Support #39

Drop PHP 7, add PHP 8.2 Support #39

Conversation

rogervila commented Jan 10, 2023

Description

rogervila commented Jan 11, 2023

gsteel left a comment

Choose a reason for hiding this comment

weierophinney commented Jan 11, 2023

rogervila commented Jan 12, 2023

gsteel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogervila commented Jan 13, 2023

rogervila commented Jan 16, 2023

weierophinney left a comment

Choose a reason for hiding this comment

rogervila commented Jan 26, 2023 • edited Loading

jonkerw85 commented Apr 20, 2023

rchrdkn commented Jun 23, 2023

rogervila commented Jun 23, 2023

rogervila commented Jan 26, 2023 •

edited

Loading