Add PHP 8.0 support

[Slamdunk]

Close #73

[Slamdunk]

The failing tests show an issue within laminas-code, I'll try to update that library first

[Slamdunk]

Yup, laminas-code needs update for PHP 8.0: https://travis-ci.com/github/laminas/laminas-code/jobs/403158984

[froschdesign]

@Slamdunk
Great work! 👍

[Slamdunk]

In the latest commit 7eb79bf I've dropped support to laminas-servicemanager < 3.4, hope it's ok for you

[froschdesign]

LGTM 👍

[boesing]

In the latest commit 7eb79bf I've dropped support to laminas-servicemanager < 3.4, hope it's ok for you

You mean laminas-code < 3.4, don't you?

[Slamdunk]

You mean laminas-code < 3.4, don't you?

No, both, take a look at the split diff of require-dev section in composer.json:

[boesing]

Overall, good addition here. Thanks so far!

Please keep the BC in mind and re-evaluate the changes in the FormElementManagerFactory to avoid bc breaks for end users.

[boesing]

Changing the interface is a BC break aswell.
Unless we are bumping major version here, we should avoid such changes.

[boesing]

Thanks for implementing BC!

Just one more thing to reconsider.

[boesing]

Oh, didn't see that.
So you've renamed the v2 plugin manager instead of the v3 one.

Please diff with the v3 plugin manager from 2.15.0 to see whats missing in here.

[Slamdunk]

So you've renamed the v2 plugin manager instead of the v3 one.

No, I've renamed the v3 one, don't know why git tells otherwise. It already is BC, no API changes introduced except for protected $shareByDefault = false; which can't work anyway with v3:

$ git diff 2.16.x:src/FormElementManager/FormElementManagerV3Polyfill.php php_80_support:src/FormElementManager.php
diff --git a/src/FormElementManager/FormElementManagerV3Polyfill.php b/src/FormElementManager.php
index aaa8202d..e9af6a3a 100644
--- a/src/FormElementManager/FormElementManagerV3Polyfill.php
+++ b/src/FormElementManager.php
@@ -6,10 +6,11 @@
  * @license   https://github.com/laminas/laminas-form/blob/master/LICENSE.md New BSD License
  */

-namespace Laminas\Form\FormElementManager;
+namespace Laminas\Form;

 use Interop\Container\ContainerInterface;
 use Laminas\Form\Element;
+use Laminas\Form\Exception;
 use Laminas\Form\ElementFactory;
 use Laminas\Form\ElementInterface;
 use Laminas\Form\Fieldset;
@@ -32,10 +33,8 @@ use function sprintf;
  *
  * Enforces that elements retrieved are instances of ElementInterface.
  */
-class FormElementManagerV3Polyfill extends AbstractPluginManager
+class FormElementManager extends AbstractPluginManager
 {
-    use FormElementManagerTrait;
-
     /**
      * Aliases for default set of helpers
      *
@@ -277,13 +276,6 @@ class FormElementManagerV3Polyfill extends AbstractPluginManager
      */
     protected $sharedByDefault = false;

-    /**
-     * Don't share form elements by default (v2)
-     *
-     * @var bool
-     */
-    protected $shareByDefault = false;
-
     /**
      * Interface all plugins managed by this class must implement.
      * @var string
@@ -399,4 +391,62 @@ class FormElementManagerV3Polyfill extends AbstractPluginManager

         return $this;
     }
+    /**
+     * Try to pull hydrator from the creation context, or instantiates it from its name
+     *
+     * @param  string $hydratorName
+     * @return mixed
+     * @throws Exception\DomainException
+     */
+    public function getHydratorFromName($hydratorName)
+    {
+        $services = $this->creationContext;
+
+        if ($services && $services->has('HydratorManager')) {
+            $hydrators = $services->get('HydratorManager');
+            if ($hydrators->has($hydratorName)) {
+                return $hydrators->get($hydratorName);
+            }
+        }
+
+        if ($services && $services->has($hydratorName)) {
+            return $services->get($hydratorName);
+        }
+
+        if (! class_exists($hydratorName)) {
+            throw new Exception\DomainException(sprintf(
+                'Expects string hydrator name to be a valid class name; received "%s"',
+                $hydratorName
+            ));
+        }
+
+        $hydrator = new $hydratorName;
+        return $hydrator;
+    }
+
+    /**
+     * Try to pull factory from the creation context, or instantiates it from its name
+     *
+     * @param  string $factoryName
+     * @return mixed
+     * @throws Exception\DomainException
+     */
+    public function getFactoryFromName($factoryName)
+    {
+        $services = $this->creationContext;
+
+        if ($services && $services->has($factoryName)) {
+            return $services->get($factoryName);
+        }
+
+        if (! class_exists($factoryName)) {
+            throw new Exception\DomainException(sprintf(
+                'Expects string factory name to be a valid class name; received "%s"',
+                $factoryName
+            ));
+        }
+
+        $factory = new $factoryName;
+        return $factory;
+    }
 }

[boesing]

hm, as I've looked into it yesterday, it looked like there were missing some aliases 🤔
I'll check that again when I find spare time.
Thanks for re-checking.

[Slamdunk]

@boesing may I help further on this?

[Slamdunk]

So, laminas-code:v4 has been released dropping support for Annostation scanning.

Libraries that rely on laminas-form that want to support PHP 8 are basically forced to upgrade to laminas-code:v4, since real test cases simply don't work on PHP 8, see for example https://github.com/doctrine/DoctrineORMModule/pull/650/checks?check_run_id=1692052713

The only solution I see to this is to upgrade laminas-form to laminas-code:v4 too, which means a BC break, hence a new MAJOR, because API like https://github.com/laminas/laminas-form/blob/2.16.x/src/Annotation/AnnotationBuilder.php#L118 are going to break.

I'm going to work on this, but I'm not sure I can replace Annotation parsing.

[Slamdunk]

In order to not reinvent the wheel, I'll try to use https://github.com/doctrine/annotations

[Slamdunk]

After a day thinking about it, the issues affecting PHP 8 and laminas-form are only related to annotations, which is not a mandatory dependency.

So we can ship this as-is, declaring partial support for annotations on PHP 8. and then we can focus on the next MAJOR with a replacement for laminas-code's annotations

[charma]

In order to not reinvent the wheel, I'll try to use https://github.com/doctrine/annotations

How about using php Attributes directly instead? Maybe this is an sufficiend alternative without the need of another dependency?
https://www.php.net/manual/de/language.attributes.overview.php

[froschdesign]

@charma

How about using php Attributes directly instead? Maybe this is an sufficiend alternative without the need of another dependency?

Please open a new issue report for that topic. Thanks in advance! 👍

[driehle]

I had a look into doctrine/annotations. I am quite confident that switching to that library will work. The structure of the annotations can stay the same, which I'd say is the most important thing. However, the signature of the annotations classes has to change slightly, for instance:

An annotation @Required(true) becomes:

• with laminas/laminas-code: new Required(['value' => true])
• with doctrine/annotations: new Required(true)

Hence, the constructors of all annotations classes have to be changed which might be considered a BC break, since those classes have not been marked as @internal. Maybe we could change the constructors from

    public function __construct(array $data)
    {
        [...]
    }

to the following:

    public function __construct($data)
    {
        if (!is_array($data) && !isset($data['value'])) {
            $data = ['value' => $data];
        }

        [...]
    }

Still, in the unlikely event that someone was extended the annotation classes and overwritten the constructor, things will likely break, as with doctrine/annotations the constructors of the annotation classes are simply invoked differently.

In consequence, it might be easier to release a new major (i.e. 3.0) with annotations support through doctrine/annotations. Additionally, support for PHP 8 attributes can be added later with a new minor (e.g. 3.1). What do you think?

[boesing]

I wonder if its just possible to use doctrine/annotations DocParser to Interpret the annotations but instantiating with the old "API" somehow?
This way, we could avoid BC breaks.

[Slamdunk]

Ok, I've verified again this PR against doctrine/DoctrineORMModule#661 and all tests pass.

I consider this ready-to-merge as users can run this on PHP 8.0 without BC Breaks, but of course we need to deprecate the current Annotation stuff. We can postpone that until we find a proper alternative.