Drop support for PHP 7.4 #2515

alcaeus · 2023-03-24T10:01:43Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

This PR drops support for PHP 7.4. This is mainly required due to the aggregation pipeline builder needing to enforce static types which cannot be worked around (see #2514).

Contained in this PR is a commit that also enforces PHP 8.0 syntax in phpcs. All changes were made using phpcbf and should be safe, but I can drop the commit if we want to defer this to a later PR.

malarzm

Changes in phpstan-baseline.neon are worrying me

malarzm · 2023-03-24T11:54:16Z

lib/Doctrine/ODM/MongoDB/Mapping/Annotations/AlsoLoad.php

    /** @var string|null */
    public $name;

    /** @param string|string[] $value */
-    public function __construct($value, ?string $name = null)
+    public function __construct(public $value, ?string $name = null)


I wonder why $name wasn't promoted as well

I assume because the property did not have a type associated with it.

since the class is final, I guess we could add types to these properties? A user might have set these properties to another type, but I guess it wouldn't work

Actually, the type is wrong: $name can be a string or list of strings, and the AnnotationDriver always converts it to an array. I've removed the parameter type.

Correction: $value is what I was talking about, $name is indeed a string. I've re-added the correct type.

malarzm · 2023-03-24T11:55:17Z

lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Document.php

        ?string $repositoryClass = null,
        array $indexes = [],
        bool $readOnly = false,
        ?string $shardKey = null,
-        $writeConcern = null
+        public $writeConcern = null,


maybe a bug in PHPCSFixer? :)

Nope, a result of phpcs requiring a trailing comma for multi-line method declarations.

Poor communication is on me, this was meant to follow previous comment on promoting only some properties instead of all 👼

malarzm · 2023-03-24T11:57:20Z

lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php

@@ -234,7 +233,7 @@ public function prepareUpdateData($document)
     */
    public function prepareUpsertData($document)


Just a note: we should revisit adding object typehint. For instance here the builder is marked as @internal so we're free to break inheritance here according to our rules

Yes, absolutely. I'd do so in a separate pull request.

alcaeus · 2023-03-24T12:08:54Z

Changes in phpstan-baseline.neon are worrying me

They look a lot less dramatic once we ignore whitespace changes. Here's the output I had before regenerating the baseline:

 ------ ------------------------------------------------------------------------------------------------------------------ 
  Line   lib/Doctrine/ODM/MongoDB/UnitOfWork.php                                                                           
 ------ ------------------------------------------------------------------------------------------------------------------ 
  1955   Unable to resolve the template type T in call to method Doctrine\ODM\MongoDB\DocumentManager::getClassMetadata()  
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unable-to-resolve-template-type                            
 ------ ------------------------------------------------------------------------------------------------------------------ 

 ------ ------------------------------------------------------------------------------------------------------------------ 
  Line   lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php                                                        
 ------ ------------------------------------------------------------------------------------------------------------------ 
  165    Unable to resolve the template type T in call to method Doctrine\ODM\MongoDB\DocumentManager::getClassMetadata()  
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unable-to-resolve-template-type                            
  193    Unable to resolve the template type T in call to method Doctrine\ODM\MongoDB\DocumentManager::getClassMetadata()  
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unable-to-resolve-template-type                            
  221    Unable to resolve the template type T in call to method Doctrine\ODM\MongoDB\DocumentManager::getClassMetadata()  
         💡 See: https://phpstan.org/blog/solving-phpstan-error-unable-to-resolve-template-type                            
 ------ ------------------------------------------------------------------------------------------------------------------

franmomu · 2023-03-25T00:26:44Z

lib/Doctrine/ODM/MongoDB/Aggregation/Aggregation.php

    /**
     * @param array<string, mixed> $pipeline
     * @param array<string, mixed> $options
     * @psalm-param PipelineExpression $pipeline
     */
-    public function __construct(DocumentManager $dm, ?ClassMetadata $classMetadata, Collection $collection, array $pipeline, array $options = [], bool $rewindable = true)
+    public function __construct(private DocumentManager $dm, private ?ClassMetadata $classMetadata, private Collection $collection, array $pipeline, private array $options = [], private bool $rewindable = true)


I've opened slevomat/coding-standard#1537

It has been merged and released, so updating slevomat/coding-standard and running phpcbf should fix this and other files.

lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/AbstractBucket.php

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/AbstractReplace.php

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GraphLookup.php

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/UnionWith.php

franmomu · 2023-03-25T00:44:31Z

lib/Doctrine/ODM/MongoDB/Event/DocumentNotFoundEventArgs.php

    private bool $disableException = false;

    /** @param mixed $identifier */
-    public function __construct(object $document, DocumentManager $dm, $identifier)
+    public function __construct(object $document, DocumentManager $dm, private $identifier)


note for another PR, we can add mixed types

franmomu · 2023-03-25T00:53:02Z

lib/Doctrine/ODM/MongoDB/Mapping/Annotations/AlsoLoad.php

    /** @var string|null */
    public $name;

    /** @param string|string[] $value */
-    public function __construct($value, ?string $name = null)
+    public function __construct(public $value, ?string $name = null)


since the class is final, I guess we could add types to these properties? A user might have set these properties to another type, but I guess it wouldn't work

lib/Doctrine/ODM/MongoDB/SchemaManager.php

tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php

franmomu

I'll try to take a look at phpstan if I find some time

lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php

* Drop support for PHP 7.4, require PHP 8.0 * Enforce PHP 8 syntax in phpcs * Fix static analysis errors * Move psalm configuration to .dist file * Set psalm phpVersion through config file * Run PHPStan on PHP 8.2 * Remove superfluous assertion

* 2.6.x: Add $search stage to aggregation pipeline builder (#2516) Support new aggregation operators in builder (#2514) Drop support for PHP 7.4 (#2515) Support new aggregation pipeline stages in builder (#2513)

alcaeus added the Enhancement label Mar 24, 2023

alcaeus added this to the 2.6.0 milestone Mar 24, 2023

alcaeus self-assigned this Mar 24, 2023

alcaeus force-pushed the drop-php-7.4 branch from d642362 to 8f96958 Compare March 24, 2023 10:07

alcaeus requested review from malarzm, IonBazan and franmomu March 24, 2023 10:15

alcaeus force-pushed the drop-php-7.4 branch from 8f96958 to 7ef28f7 Compare March 24, 2023 11:37

malarzm reviewed Mar 24, 2023

View reviewed changes

alcaeus requested a review from malarzm March 24, 2023 13:51

Drop support for PHP 7.4, require PHP 8.0

0dc981f

alcaeus force-pushed the drop-php-7.4 branch 2 times, most recently from 7554ff5 to e7752c6 Compare March 24, 2023 14:45

franmomu reviewed Mar 25, 2023

View reviewed changes

alcaeus force-pushed the drop-php-7.4 branch from e7752c6 to b2ec057 Compare March 27, 2023 07:27

alcaeus requested a review from franmomu March 27, 2023 07:28

alcaeus added 5 commits March 28, 2023 10:10

Enforce PHP 8 syntax in phpcs

831a820

Fix static analysis errors

815b5f9

Move psalm configuration to .dist file

64e529c

Set psalm phpVersion through config file

8655316

Run PHPStan on PHP 8.2

36be437

alcaeus force-pushed the drop-php-7.4 branch from b2ec057 to 36be437 Compare March 28, 2023 08:12

alcaeus mentioned this pull request Mar 28, 2023

Add $search stage to aggregation pipeline builder #2516

Merged

franmomu approved these changes Mar 28, 2023

View reviewed changes

lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php Outdated Show resolved Hide resolved

Remove superfluous assertion

505cd49

alcaeus force-pushed the drop-php-7.4 branch from e84a9c1 to 505cd49 Compare March 28, 2023 09:03

alcaeus merged commit e235642 into doctrine:2.6.x Mar 28, 2023

alcaeus deleted the drop-php-7.4 branch March 28, 2023 09:15

franmomu mentioned this pull request Apr 5, 2023

Fix psalm build #2525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for PHP 7.4 #2515

Drop support for PHP 7.4 #2515

alcaeus commented Mar 24, 2023

malarzm left a comment

malarzm Mar 24, 2023

alcaeus Mar 24, 2023

franmomu Mar 25, 2023

alcaeus Mar 27, 2023

alcaeus Mar 28, 2023

malarzm Mar 24, 2023

alcaeus Mar 24, 2023

malarzm Mar 28, 2023

malarzm Mar 24, 2023

alcaeus Mar 24, 2023

alcaeus commented Mar 24, 2023

franmomu Mar 25, 2023

franmomu Mar 25, 2023

alcaeus Mar 27, 2023

franmomu Mar 25, 2023

franmomu Mar 25, 2023

franmomu left a comment

		@@ -234,7 +233,7 @@ public function prepareUpdateData($document)
		*/
		public function prepareUpsertData($document)

Drop support for PHP 7.4 #2515

Drop support for PHP 7.4 #2515

Conversation

alcaeus commented Mar 24, 2023

Summary

malarzm 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

alcaeus commented Mar 24, 2023

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

franmomu left a comment

Choose a reason for hiding this comment