Bump to minimum PHP 7.2

[DannyvdSluijs]

As mentioned in several issues and confirmed by @Seldaek we can and should bump the minimum PHP level to >= 7.2

I've started this discussion to first discuss and collect the work to be done before taking action. Also releasing v6.0.0 has a higher priority compared to this.

Things todo

• Bump composer.json from php: >= 5.3.3 to php: >= 7.2, see Extend pipeline for newer versions php #746
• Use language features
  • Short array notation (PHP 5.4), see refactor: Replace traditional syntax array with short syntax array #747
  • Visibility modifiers on class constants, see refactor: add visibility modifiers to class constants #757
  • Null coalescing operator (PHP 7.0)
  • Spaceship operator (PHP 7.0)
  • Scalar parameter types & return type hints (PHP 7.0)
  • Generators (PHP 7.0)
  • Nullable types (PHP 7.1)
  • Iterable and Void return type (PHP 7.1)
  • Multi catch (PHP 7.1)
  • Object return types (PHP 7.2)
  • Trailling comma's
  • Removal of @param where possible
• Update dependencies
  • phpunit/phpunit: ^8.5.* which also requires to update the test suite, adding void return types on the tests, see Extend pipeline for newer versions php #746
  • friendsofphp/php-cs-fixer: 3.2.1
  • marc-mabe/php-enum constraint can be adjusted to ^4.0 (as 2.* and 3.* required a PHP version for which we are dropping support.)
• Add declare (strict_types=1) see refactor: add declare strict_types to all classes in ./src #758
• Expand build matrix to include newer PHP version and exclude older unsupported versions, see Extend pipeline for newer versions php #746
• Fix reported issues
  • Fix ObjectIterator for PHP8 #682, see Extend pipeline for newer versions php #746
  • [PHP 8.4] Fixes for implicit nullability deprecation #717, see Extend pipeline for newer versions php #746

Language features we can't use

• Typed properties (requires PHP 7.4)
• mixed type (Requires PHP 8.0)
• Union types (Requires PHP 8.0)
• Enums (requires PHP 8.1)

One other PR to consider is #689

[erayd]

One point to note on the dependency updates: php-cs-fixer version changes (even minor version changes that are supposed to be compatible) have on multiple past occasions also been associated with changes in its behaviour that have caused it to make unwanted changes to the logic of this library's code, not just presentation. Any update of php-cs-fixer should also be combined with careful review of any changes it proposes to make to the existing codebase when run to ensure it isn't introducing logic bugs - the logic problems can be subtle.

This is the reason why it's pinned to an exact version, rather than allowing any compatible version.

[nicolas-grekas]

releasing v6.0.0 has a higher priority compared to this.

May I ask this to be reconsidered?
At the moment, all PHP 8.4 builds are spammed because of this library, see e.g. https://github.com/symfony/polyfill/actions/runs/9582333371/job/26421086573 for what I mean.

[erayd]

@nicolas-grekas Changing the PHP minimum version is a breaking change; the library can't reasonably be released with that change under a 5.x or 6.x version number.

The plan is to release 6.0 ASAP (with support for PHP >= 5.3), then immediately deprecate it. This gives those who are currently running the master branch in production an actual version to pin to if needed, and allows us to move forward with breaking changes like the version bump in v7.

[Seldaek]

Changing the PHP minimum version is a breaking change

I don't mind the current plan we have here and don't want to reopen that debate, but just for the record this statement really isn't correct IMO. Breaking API from 5.1 to 5.2 would be a breaking change for sure, but bumping a required dependency isn't one, as if the dependency upgrade cannot be satisfied the Composer solver will not upgrade to 5.2. The PHP dependency isn't really more special than any other. And for example I don't think you would consider upgrading Composer's dependency on this library from 5.x to 6.x to be a BC break?

[erayd]

...but bumping a required dependency isn't one, as if the dependency upgrade cannot be satisfied the Composer solver will not upgrade to 5.2

I would normally agree with you, but I consider it a breaking change in this particular case, because it would mean that people who can't cope with it would be permanently locked out of security upgrades on the 5.x branch. PHP isn't a typical dependendency - it's the whole operating environment, not just a library. Which IMO is a big part of why breaking changes require a major version bump in the first place - it means you don't just leave the legacy folks hanging. It's not just about API compatibility.

I don't consider upgrading composer's dependency on it to be breaking, because we know composer can cope with it. But there are unfortunately plenty of legacy projects out there which can't. The plan we have (bumping the PHP dependency in v7) means that in the event of a serious security issue, we'll still be able to address it for those folks stuck on 5 & 6.

[Seldaek]

Ok I've now went ahead and done that release quickly https://github.com/jsonrainbow/json-schema/releases/tag/5.3.0

It's just the minimal changes to make it work on 8.4, but this is by no means bringing 5.x up to speed with all the features of PHP 7.1, because I don't think that is worth the effort.

Let's focus on v6 now, @DannyvdSluijs I think we could do the same there, tag 6.0 then work on 6.1 bumping PHP and doing all the changes, unless there are some BC breaks worth doing to support php7.2+ then we can also just call it 7.0 after. Same same..

[DannyvdSluijs]

For now I've created a branch 6.0.x in case we need to a patch release. This now allows the main branch to be used for either 6.1 or 7.0.

I know many people are willing to help out but still are stuck for time (no blaming, just an honest observation from my perspective). I'll leave PR's open for some time to allow for feedback and tag them Awaiting feedback but if the PR isn't touching the business logic I would prefer to merge them even if left without feedback. I'm curious what you guys think? I've already done a couple of really small fixes this way (#741, #743 and #745)

Currently #746 is such a PR that I would merge if no feedback is provided within a set amount of time.

[jrfnl]

@Seldaek Any plans on packaging a new Composer PHAR with the updated (5.3.0) dependency ?

[Seldaek]

@jrfnl yeah.. I've been trying to get there since early July 😅 Summer break got in the way, but it's definitely on my radar.

[jrfnl]

@Seldaek I feel you. Hope you had a good break and looking forward to the newly packaged PHAR once you get to it ;-)

[marineusde]

I think, the php-requirement should be PHP >= 8.1, cause older versions are not in the security support: https://www.php.net/supported-versions.php . If someone dont want to upgrade his own work with a little refactoring, he can use older versions of the package.

Other suggestions:

• use types in parameters and return values, there is no need to use @param
• use phpstan

If you need somebody to do that, I can help :)

[DannyvdSluijs]

Thanks for the inputs. The reasoning behind targeting PHP >= 7.2 is to keep it compatible with the biggest dependant we are having which is Composer (the PHP package manager).

We are closing to releasing v6.0.0 of this library and this topic is the next one on the list. The changes (in amount of lines) is going to be rather big so I was planning to go in steps (see the todolist above.) If you have any concrete additions or concerns to them that would be helpful . For now Ill add the removal of @param where possible and phpstan to the list.

[marineusde]

thank you for that. I checked out version 6.0 and install phpstan and configure php 8.2 locally.

after running phpstan with the configured level 2 I just found errors in this file and fixed it:

<?php

/*
 * This file is part of the JsonSchema package.
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace JsonSchema\Iterator;

/**
 * @package JsonSchema\Iterator
 *
 * @author Joost Nijhuis <jnijhuis81@gmail.com>
 */
class ObjectIterator implements \Iterator, \Countable
{
    private object $object;

    private int $position = 0;

    private array $data = array();

    private bool $initialized = false;

    public function __construct(object $object)
    {
        $this->object = $object;
    }

    public function current(): mixed
    {
        $this->initialize();

        return $this->data[$this->position];
    }

    public function next(): void
    {
        $this->initialize();
        $this->position++;
    }

    public function key(): int
    {
        $this->initialize();

        return $this->position;
    }

    public function valid(): bool
    {
        $this->initialize();

        return isset($this->data[$this->position]);
    }

    public function rewind(): void
    {
        $this->initialize();
        $this->position = 0;
    }

    public function count(): int
    {
        $this->initialize();

        return count($this->data);
    }

    /**
     * Initializer
     */
    private function initialize(): void
    {
        if (!$this->initialized) {
            $this->data = $this->buildDataFromObject($this->object);
            $this->initialized = true;
        }
    }

    private function buildDataFromObject(object $object): array
    {
        $result = array();

        $stack = new \SplStack();
        $stack->push($object);

        while (!$stack->isEmpty()) {
            $current = $stack->pop();
            if (is_object($current)) {
                array_push($result, $current);
            }

            foreach ($this->getDataFromItem($current) as $propertyName => $propertyValue) {
                if (is_object($propertyValue) || is_array($propertyValue)) {
                    $stack->push($propertyValue);
                }
            }
        }

        return $result;
    }

    private function getDataFromItem(object|array $item): object|array
    {
        if (!is_object($item) && !is_array($item)) {
            return array();
        }

        return is_object($item) ? get_object_vars($item) : $item;
    }
}

[DannyvdSluijs]

I've done a comparison between the version on my branch where I'm working on bumping the minimum PHP version.
You're introducing features which can't be used as the minimum PHP version will still be PHP 7.2 such as Typed properties, mixed type, union types

What remains are some return types being added. At this point I would like to keep the initial PR as small as possible with the focus on raising the minimum PHP version and getting the pipeline working for the version we will be supporting (7.2, 7.3, 7.4, 8.0, 8.1, 8.2and8.4`

Once we crossed that bridge we can start working on the item list above.

[DannyvdSluijs]

I started working on a Draft PR to discover the impact of of supporting PHP 8.0 officially and drop support for PHP >= 7.1.
First goal is to include PHP 8.0 and greater in the pipeline in order to have insights on the effect of consecutive changes and existing pull requests.

You can check the progress at #746 this only reflects the latest achieved commit and I might have other changes pending on my local computer.