Prepare develop branch to target PHP 7.2 #80

weierophinney · 2018-11-19T22:58:57Z

To prepare for the 3.0 release (already begun with #14), this patch ups the minimum supported version of PHP to 7.2, and adds type hints, including object typehints. Additionally, a number of interfaces were written such as to allow optional parameters, but did not define them in the interface; this patch updates those to add the optional values as nullable value types.

PHP 7.2 was chosen as a target as it adds the object typehint, which helps us firm up the interfaces in this particular component tremendously, as extract() can now require an object, and hydrate() can both require accepting one as well as returning one.

TODO

vaclavvanik · 2018-11-20T06:52:36Z

I would like to incorporate small update of DateTimeFormatterStrategy as I consulted with @froschdesign.

Should I wait when this PR will be merged?

Second I would to create DateTimeImmutableFormatterStrategy.

weierophinney · 2018-11-20T13:45:23Z

@vaclavvanik I already merged that particular PR to the develop branch, on which this is based; if there are any additions you want to make to it, let me know!

As for an additional strategy - that can happen either after this is merged and before release, or as part of a 3.1.0 release (which we can do pretty quickly following).

vaclavvanik · 2018-11-20T14:00:25Z

@weierophinney please update DateTimeFormatterStrategy::hydrate method to:

/**
 * Converts date time string to DateTime instance for injecting to object
 *
 * {@inheritDoc}
 *
 * @param null|string|DateTimeInterface $value
 * @param  array $data  The whole data is optionally provided as context.
 * @return null|string|DateTimeInterface
 * @throws Exception\InvalidArgumentException
 */
public function hydrate($value, ?array $data = null)
{
    if ($value === '' || $value === null || $value instanceof DateTimeInterface) {
        return $value;
    }

    if (! is_string($value)) {
        throw new Exception\InvalidArgumentException(sprintf(
            'Unable to hydrate. Expected null, string or DateTimeInterface. %s was given.',
            is_object($value) ? get_class($value) : gettype($value)
        ));
    }

    $hydrated = DateTime::createFromFormat($this->format, $value, $this->timezone);

    if ($hydrated === false && $this->dateTimeFallback) {
        $hydrated = new DateTime($value, $this->timezone);
    }

    return $hydrated ?: $value;
}

- Require at least PHP 7.1 - Require 3.X (or latest) versions of ZF components. - Require PHPUnit 7.4

- Added `declare(strict_types=1)` to all classes - Adds typehints to all parameters and return values. In addition, it expands interfaces that were allowing optional values to provide appropriate typehints. Also uses `object` typehints (nullable in some cases) where appropriate: - `HydrationInterface` - `ExtractionInterface` - `NamingStrategyInterface` - `StrategyInterface` - All classes implementing the above - Calls to `getNamingStrategy()` must be preceded by `hasNamingStrategy()`, to ensure the method does not attempt to return `null` (which is now disallowed by the interface). - Removes type check/throw combinations where typehints now make them obsolete. - Updates test expectations and/or removes tests to correspond with these changes. - Removes data provider entries that are no longer relevant. - Removes annotations made redundant by typehints. - Adds typehints for previously "mixed" types when we now know the type will not vary (e.g., `Reflection::getReflProperties()` can typehint `$input` against `object` now). - Fixes `ClosureStrategy` to check if the composed extraction/hydration callback is actually set before attempting to call it; if not set, it returns the value verbatim instead. - Fixes the `ExplodeStrategy` to cast the `$value` to `string` during hydration when calling `explode()`. This preserves previous behavior. - Removes tests against zend-servicemanager v2 (affects `DelegatingHydratorFactory` and `HydratorPluginManager` and related factory.

- Return value verbatim when empty string, null, or a `DateTimeInterface` instance. - Raise an exception for other non-string values. Also modified the case when hydration fails to vary how the `DateTime` instance is created based on whether or not a timezone is present.

weierophinney · 2018-11-20T16:33:21Z

@vaclavvanik Made the changes requested, along with additional unit tests.

- Mark the "Home" page - Remove the copyright configuration; no longer needed - Sync site_description to package descripiton

vaclavvanik · 2018-11-20T17:11:21Z

@weierophinney awesome. +1

vaclavvanik · 2018-11-20T17:56:44Z

@weierophinney I looked at bakura v3 hydrator for inspiration when v3 is comming.

Bakura created Hydrator\NamingStrategy\ProvidesNamingStrategyTrait and Hydrator\Strategy\ProvidesStrategyTrait. It looks it simplifies creating own hydrators (which does not extend AbstractHydrator).

thomasvargiu · 2018-11-21T12:16:28Z

@weierophinney Could be useful to add phpstan in the CI for this kind of changes? It's useful when using type hints.

Ocramius · 2018-11-21T12:46:07Z

I asked for something like this specifically (even level 7) before going on and adding strict types to zend-di, since types are also runtime assertions in this language.

…

On Wed, 21 Nov 2018, 13:16 Thomas Mauro Vargiu ***@***.*** wrote: @weierophinney <https://github.com/weierophinney> Could be useful to add phpstan in the CI for this kind of changes? It's useful when using type hints. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakLD_Ste295nDxljKFVOxaVRjTezYks5uxUQdgaJpZM4YqDXD> .

- Adds phpstan as a development requirement - Adds phpstan configuration at level 7, scanning the src tree - Incorporates phpstan into the Travis CI workflow

- A number or properties are stored as `ArrayObject` instances, but we were performing various unsupported array operations on them. - Adds zend-modulemanager as a dependency, to eliminate issues with the `Module` class. - Some property annotations were incorrect; updating them to allow known types fixed errors. - Some parameter annotations were incorrect. - Raise an exception from `getNamingStrategy()` if no naming strategy is present. - Subclasses should generally call the parent constructor. - Replace `is_callable()` with `method_exists()` everywhere. - Remove casting during assignments when the type is known. - Do `$serializer` assignment within `SerializableStrategy::getSerializer()` to prevent incorrect detection of string as a type for `$serializer` after lazy loading I've added three `ignoreErrors` directives: - One property is set correctly, and assignments are correct, but phpstan cannot seem to determine that an assignment is valid. - We are passing a PSR-11 container to plugin managers, which is always valid within this context (it will also fulfill a typehint the plugin manager accepts in all situations), but phpstan does not know the context, so assumes an error is present. - A number of `||` and `&&` conditionals are flagged as always returning true or false, but they can actually vary.

weierophinney · 2018-11-26T18:34:36Z

@Ocramius Just pushed a changeset that includes phpstan checks. I had to create a few exclusions (<rant>could not figure out valid YAML to make the exclusions file-specific</rant>), but surprisingly few; the commit details which and why.

weierophinney · 2018-11-26T18:37:26Z

Bakura created Hydrator\NamingStrategy\ProvidesNamingStrategyTrait and Hydrator\Strategy\ProvidesStrategyTrait. It looks it simplifies creating own hydrators (which does not extend AbstractHydrator).

We can add these in a later patch; right now, focusing on getting the existing functionality updated.

- Creates both `v2/` and `v3/` trees in the documentation, based on 2.4.1 docs. - Makes all existing docs into redirects, redirecting to the v2 docs. - Updates the `mkdocs.yml`: - Makes the top-level navigation items point to the v3 artifacts. - Adds a `v2` navigation item, with subitems pointing to the v2 docs. - Adds hidden pages pointing to the legacy locations, to allow redirects. **NOTE**: No `v1/` tree is created, as that version did not have ship documentation within the repository.

mkdocs.yml

froschdesign · 2018-11-26T21:15:10Z

@weierophinney
I'm working on the documentation content. I have already written code examples for all hydrators and most of the strategies. Maybe I'm ready in the next two days, but you do not have to wait with the release of version 3.

Both `ExtractEvent` and `HydrateEvent` were defining `object` parameters and return types via annotations, but could easily define them explicitly as typehints.

Allows greater compatibility with installs of mkdocs.

weierophinney · 2018-11-28T14:50:10Z

I also saw that many native functions are not imported.

Can you expand on this? I did a thorough audit yesterday. I did not import functions in the test suite because it's basically unnecessary; however, the src/ tree should be fully covered at this point.

I did another pass on the src/ tree and found a handful of unimported global functions. I'm fairly confident they're all imported now.

thomasvargiu

Little improvements in code. Nothing important.

What should we do with phpdoc? Are we using it partially because we are using type hints?
There are many methods with no phpdoc @param (or @return) or with partial declared parameters. Using php-cs-fixer or PhpStorm inspections can help to identify them.

src/Strategy/ExplodeStrategy.php

src/AbstractHydrator.php

src/Exception/InvalidCallbackException.php

src/Iterator/HydratingArrayIterator.php

src/NamingStrategy/ArrayMapNamingStrategy.php

src/NamingStrategy/CompositeNamingStrategy.php

src/NamingStrategy/UnderscoreNamingStrategy/UnderscoreToCamelCaseFilter.php

Since we have nullables now, we know it will be exactly null or an integer.

They were defined in the parent `HydratingIteratorIterator`

We were capturing to the end of the string already.

src/NamingStrategy/UnderscoreNamingStrategy/UnderscoreToCamelCaseFilter.php

src/NamingStrategy/CompositeNamingStrategy.php

src/NamingStrategy/NamingStrategyInterface.php

src/NamingStrategy/UnderscoreNamingStrategy.php

src/Strategy/DateTimeFormatterStrategy.php

src/Filter/OptionalParametersFilter.php

src/Iterator/HydratingIteratorIterator.php

Incorporates all feedback except ensuring license docblocks are up-to-date with regards to format.

src/NamingStrategy/NamingStrategyInterface.php

weierophinney · 2018-11-28T19:52:35Z

@thomasvargiu —

What should we do with phpdoc? Are we using it partially because we are using type hints?
There are many methods with no phpdoc @param (or @return) or with partial declared parameters. Using php-cs-fixer or PhpStorm inspections can help to identify them.

We do not need to declare annotations for parameters or return values that have type hints, unless we want to add descriptions to them; otherwise, they are redundant. PHPDoc and IDEs will pick up the declared typehints when no annotations exist. (They will also correctly mix-and-match declared typehints with annotations provided in order to provide the full signature.)

We use phpcs, but do not currently have a ruleset that will populate them; IIRC, v2 of our ruleset will include this.

thomasvargiu · 2018-11-28T20:03:47Z

It just was a question to know it. Default PhpStorm inspections give a warning about them (I’m not saying it’s correct).
It’s not a problem but it’s weird to see that when a method has 4 parameters, only 3 of them have phpdoc @param. I would like to try to generate php documentation to see what result is.

Ocramius · 2018-11-28T20:04:55Z

Generated docs are completely useless

…

On Wed, 28 Nov 2018, 21:03 Thomas Mauro Vargiu ***@***.*** wrote: It just was a question to know it. Default PhpStorm inspections give a warning about them (I’m not saying it’s correct). It’s not a problem but it’s weird to see that when a method has 4 parameters, only 3 of them have phpdoc @param <https://github.com/param>. I would like to try to generate php documentation to see what result is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakAxycW5OmBhXYPeEf9GImb0Y4jyqks5uzuwjgaJpZM4YqDXD> .

Referencing a function by string automatically assumes it is globally qualified.

In English, we typically sort such that letters have precedence over symbols; however, in computers, this is evidently exactly the opposite. Since computers do the automation, we go with what they do. *sigh*

Ocramius

🚢

vaclavvanik · 2018-11-28T20:09:28Z

composer.json

-        "php": "^5.6 || ^7.0",
-        "zendframework/zend-stdlib": "^3.0"
+        "php": "^7.2",
+        "zendframework/zend-stdlib": "^3.2.1"


Mb_* functions are used (at least in UnderscoreNamingStrategy\CamelCaseToUnderscoreFilter.php)
Imo we should require ext-mbstring

@vaclavvanik
Please create a pull request and use the develop branch for your improvement. This PR is merged and finished.

i create it tomorrow. today i am sick :-/

We test to see if those functions are available before using them, defaulting to the non-mb variants if they are not. As such, there is no hard requirement on ext-mbstring.

…egy constructor

weierophinney added enhancement bc break labels Nov 19, 2018

weierophinney force-pushed the feature/php-7.1-support branch 2 times, most recently from 625434c to 45cde68 Compare November 20, 2018 16:25

weierophinney added 3 commits November 20, 2018 10:28

Update dependencies

b3a3de7

- Require at least PHP 7.1 - Require 3.X (or latest) versions of ZF components. - Require PHPUnit 7.4

weierophinney force-pushed the feature/php-7.1-support branch from 45cde68 to 583cd24 Compare November 20, 2018 16:28

weierophinney changed the title ~~Prepare develop branch to target PHP 7.1 or 7.2~~ Prepare develop branch to target PHP 7.2 Nov 20, 2018

Update mkdocs.yml

722b1e4

- Mark the "Home" page - Remove the copyright configuration; no longer needed - Sync site_description to package descripiton

weierophinney added this to the 3.0.0 milestone Nov 20, 2018

weierophinney added 2 commits November 26, 2018 11:20

Adds phpstan into CI toolchain

0d07c0d

- Adds phpstan as a development requirement - Adds phpstan configuration at level 7, scanning the src tree - Incorporates phpstan into the Travis CI workflow

froschdesign reviewed Nov 26, 2018

View reviewed changes

mkdocs.yml Outdated Show resolved Hide resolved

weierophinney added 4 commits November 26, 2018 16:36

Set explicit object param/return types in event implementations

8ab8be9

Both `ExtractEvent` and `HydrateEvent` were defining `object` parameters and return types via annotations, but could easily define them explicitly as typehints.

Use object return type for HydratorListener::onHydrate()

042ce54

Adds migration document for v3

fbcbebf

Revert rename of pages to nav in mkdocs.yml

6334c28

Allows greater compatibility with installs of mkdocs.

thomasvargiu reviewed Nov 28, 2018

View reviewed changes

weierophinney added 6 commits November 28, 2018 09:36

Remove unnecessary ternary when setting $explodeLimit

4850813

Since we have nullables now, we know it will be exactly null or an integer.

DomainException already implements ExceptionInterface

b6115d7

Remove unnecessary property declarations in HydratingArrayIterator

800dc12

They were defined in the parent `HydratingIteratorIterator`

Use null coalesce instead of ternary within ArrayMapNamingStrategy

7f62d77

Use null coalesce within CompositeNamingStrategy instead of ternary

125194c

Remove third argument to substr within UnderscoreToCamelCaseFilter

33e18e1

We were capturing to the end of the string already.

michalbundyra reviewed Nov 28, 2018

View reviewed changes

Incorporate feedback from @webimpress

b0babc4

Incorporates all feedback except ensuring license docblocks are up-to-date with regards to format.

michalbundyra reviewed Nov 28, 2018

View reviewed changes

src/NamingStrategy/NamingStrategyInterface.php Outdated Show resolved Hide resolved

weierophinney added 2 commits November 28, 2018 13:37

Correct typehint for NamingStrategyInterface::extract $object argument

046a2ea

Updates license docblocks for all test classes and assets

1f2e3f7

weierophinney added 2 commits November 28, 2018 15:46

Remove lcfirst import in UnderscoreToCamelCaseFilter

e48e508

Referencing a function by string automatically assumes it is globally qualified.

Sorting of imports when get_class/gettype are used

b7b5b9a

In English, we typically sort such that letters have precedence over symbols; however, in computers, this is evidently exactly the opposite. Since computers do the automation, we go with what they do. *sigh*

Ocramius self-assigned this Nov 29, 2018

Ocramius approved these changes Nov 29, 2018

View reviewed changes

Ocramius merged commit 34fe752 into zendframework:develop Nov 29, 2018

vaclavvanik reviewed Nov 29, 2018

View reviewed changes

weierophinney added a commit that referenced this pull request Nov 29, 2018

Adds CHANGELOG entries for #80

11898fc

weierophinney deleted the feature/php-7.1-support branch November 29, 2018 14:13

weierophinney mentioned this pull request Nov 29, 2018

Move *EnabledInterface definitions to appropriate namespaces #83

Merged

weierophinney added a commit to weierophinney/zend-hydrator that referenced this pull request Dec 3, 2018

Updates CHANGELOG for zendframework#80 to note private MapNamingStrat…

022a61c

…egy constructor

weierophinney added a commit to weierophinney/zend-hydrator that referenced this pull request Dec 3, 2018

Updates CHANGELOG for zendframework#80 to note private MapNamingStrat…

427ee93

…egy constructor

weierophinney added a commit to weierophinney/zend-hydrator that referenced this pull request Dec 3, 2018

Updates CHANGELOG for zendframework#80 to note private MapNamingStrat…

e18f465

…egy constructor

weierophinney mentioned this pull request Dec 4, 2018

Remove object return typehints #88

Merged

vaclavvanik mentioned this pull request Dec 7, 2018

#80 Added Zend\Hydrator\Strategy\DateTimeFormatterStrategy docs #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare develop branch to target PHP 7.2 #80

Prepare develop branch to target PHP 7.2 #80

weierophinney commented Nov 19, 2018 •

edited

Loading

vaclavvanik commented Nov 20, 2018

weierophinney commented Nov 20, 2018

vaclavvanik commented Nov 20, 2018

weierophinney commented Nov 20, 2018

vaclavvanik commented Nov 20, 2018

vaclavvanik commented Nov 20, 2018

thomasvargiu commented Nov 21, 2018

Ocramius commented Nov 21, 2018 via email

weierophinney commented Nov 26, 2018

weierophinney commented Nov 26, 2018

froschdesign commented Nov 26, 2018

weierophinney commented Nov 28, 2018

thomasvargiu left a comment •

edited

Loading

weierophinney commented Nov 28, 2018

thomasvargiu commented Nov 28, 2018

Ocramius commented Nov 28, 2018 via email

Ocramius left a comment

vaclavvanik Nov 28, 2018

froschdesign Nov 29, 2018

vaclavvanik Nov 29, 2018

weierophinney Nov 29, 2018

Prepare develop branch to target PHP 7.2 #80

Prepare develop branch to target PHP 7.2 #80

Conversation

weierophinney commented Nov 19, 2018 • edited Loading

TODO

vaclavvanik commented Nov 20, 2018

weierophinney commented Nov 20, 2018

vaclavvanik commented Nov 20, 2018

weierophinney commented Nov 20, 2018

vaclavvanik commented Nov 20, 2018

vaclavvanik commented Nov 20, 2018

thomasvargiu commented Nov 21, 2018

Ocramius commented Nov 21, 2018 via email

weierophinney commented Nov 26, 2018

weierophinney commented Nov 26, 2018

froschdesign commented Nov 26, 2018

weierophinney commented Nov 28, 2018

thomasvargiu left a comment • edited Loading

Choose a reason for hiding this comment

weierophinney commented Nov 28, 2018

thomasvargiu commented Nov 28, 2018

Ocramius commented Nov 28, 2018 via email

Ocramius left a comment

Choose a reason for hiding this comment

vaclavvanik Nov 28, 2018

Choose a reason for hiding this comment

froschdesign Nov 29, 2018

Choose a reason for hiding this comment

vaclavvanik Nov 29, 2018

Choose a reason for hiding this comment

weierophinney Nov 29, 2018

Choose a reason for hiding this comment

weierophinney commented Nov 19, 2018 •

edited

Loading

thomasvargiu left a comment •

edited

Loading