api icon indicating copy to clipboard operation
api copied to clipboard

Bump version of php

Open tacman opened this issue 4 years ago • 10 comments

PHP 7.3 will reach EOL in two weeks.

If we bump the minimum requirements to php 7.4, we can start using better typehints.

The API currently allows either an object OR an id to be passed. If we require PHP8, we can change the signature to something like

    /**
     * Get the videos that have been added to a TV season (trailers, teasers, etc...)
     */
    public function getVideos(int|Tv $tvShow, int|Season $season, array $parameters = [], array $headers = []): Videos
    {
        if ($tvShow instanceof Tv) {
            $tvShow = $tvShow->getId();
        }

I've found that adding typehints makes coding much easier.

tacman avatar Nov 20 '21 12:11 tacman

Perhaps this could be a version 5 feature. If so, we could deprecate calling these functions with IDs and require passing the appropriate objects.

tacman avatar Nov 20 '21 12:11 tacman

Might be an idea to start pushing for a 5 major release, however that should also deserve more attention than I am willing to spend time on right now :-). We might just tag a 5.0 to abandon deprecated PHP versions for time being, and possibly create a roadmap for 6.0 if people want to start improving the codebase more.

wtfzdotnet avatar Dec 11 '21 14:12 wtfzdotnet

Requiring PHP8 for version 6 would make it easier to get excited about contributing.

Also, composer.json excludes Symfony 6 now, obviously we should allow that too.

tacman avatar Dec 11 '21 15:12 tacman

The usual problem with the library has been that there aren't enough contributors to really divide up the work, I've spent quite some time for the 4.0 release again, but to get things to another level I think a lot of code has to go out the window.

The less code we have, the easier it is to maintain, and more fun for junior's to also chip in and make small but meaningful changes easily. Besides most of the code originated from like 2013, so a really really thoughtful and broad sweep would be nice, but very time consuming.

Something I've been thinking of for a while is to see how much of the stuff could be actually auto generated ( I know it sounds a little silly ), but see https://github.com/OpenAPITools/openapi-generator and https://api.stoplight.io/v1/versions/9WaNJfGpnnQ76opqe/export/oas.json. If there would be a particular set of auto generated files it would be very easy keeping up with new features added. Simply thinking we can auto-generate everything probably is an Utopia but I think there's plenty of possibilities to reduce a lot of maintenance this way.

wtfzdotnet avatar Dec 11 '21 15:12 wtfzdotnet

cc @neildaniels thoughts?

wtfzdotnet avatar Dec 11 '21 15:12 wtfzdotnet

Give this a run,

curl -X POST --header 'Content-Type: application/json' \
  --header 'Accept: application/json' \
  -d '{"openAPIUrl": "https://api.stoplight.io/v1/versions/9WaNJfGpnnQ76opqe/export/oas.json"}' \                                         
  'http://api.openapi-generator.tech/api/gen/clients/php'

Generated code is far from my likings of standards, but if there's enough room to modify the process, who knows it could be very useful.

wtfzdotnet avatar Dec 11 '21 15:12 wtfzdotnet

I'm all for dropping old PHP version support—even PHP 7.4 is security-only updates. I've already migrated my codebase to PHP 8.1.

If there's going to be any attempt at a sizable refactor, I'd say just jump to a PHP 8 baseline. I'd even argue jumping to PHP 8.1 baseline could yield a really modern library—especially with Enumerations and maybe dropping setters/getters in favor of pure properties.


I don't really have any experience using any auto-generated code, but I agree that reducing manually written boilerplate should probably be a big goal of a rewrite.

For example, in the existing library, adding anything "new" usually means touching 2–4 files (more if you're adding unit tests), and each file has about 10 lines of near-boilerplate.

If a code-generator can help reduce that—cool. If not, that's fine, but work on improving the ergonomics of having newcomers makes simple changes.

neildaniels avatar Dec 12 '21 02:12 neildaniels

Can't agree more that the idea's I had initially of how this should be written have changed over the years since 2013 lol :-).

Concerning the auto generated stuff, I also don't have experience with it but the thought of it is certainly very interesting. I'd have to fiddle around with the templates and possibilities of these generators to get it up to a level where I'd be satisfied of code quality over convenience. I think one of the most prominent problems with this approach would be the unit tests, even as of now the stored json files ( to keep testing fast ), are very outdated. Then comes the problem of making assertions on data that is vulnerable to change ( which should refresh more often if implementing this auto-generation stuff imo ).

For me that drills down to;

  • Either figuring out if we can adapt tests to changing data as well ( possibly generate these? )
  • Clap in your hands for not having to redo the boilerplate code, but bite the bullet and just write the tests ourselves

Current code then around event dispatching and handling PSR standards shouldn't need much changes, but still there's always room for improvement.

Regardless if we take that auto generation approach or not, a next major version should really deserve a good backwards compatibility break and improve as much as possible.

@neildaniels could you elaborate more on the aspect you mentioned about ergonomics, if the previous doesn't fit that glove :-)?

wtfzdotnet avatar Dec 13 '21 23:12 wtfzdotnet

Oh and just to clarify; I don't see the automation bit as the holy grail for a revamp, I just see the value it could add maintaining this library better ( easier ).

If the conclusion is that road is a dead end, no hard feelings or whatever, just one way or another I'm not going to do a rewrite on my own. Regardless of the route you'd take it is a bunch of work, and to be honest I haven't used this library personally anymore for the past 5-6 years. ( Which is also kind of why I am always late with PR's etc ).

wtfzdotnet avatar Dec 13 '21 23:12 wtfzdotnet

For example, when added Translations support to Collections, I had to touch 5 files (and a couple files for Tests).

IMO, it's not immediately obvious which 5 files were actually needed to modify. There's not really documentation and nor is the code itself very obvious.

I could be naïve, but I suspect there's an opportunity to consolidate the 5 different files into like 2. I think the "API", "Repo", and "Factory" files are all kind of unnecessary on their own.

From my perspective, I'd say this library tried to be as flexible and composable as possible—for example: if you didn't want to use the library's "Repo" classes, you could make API calls totally on your own and then hydrate those totally on your own as well.

I have doubts that this modularity is really worth it.


This is very hand-wavy, but what if a PHP 8.1 Movie model file could look like this:

<?php

class Movie
{
    public function __construct(
        public readonly int $id,
        public readonly string $title = null,
        public readonly bool $isAdult = false,
        public readonly string $imdbID = null,
        public array $translations = null,
    ) {}

    public static function createFromAPIData(array $apiData): self {
        return new static(
            id: $apiData['id'] ?? null,
            title: $apiData['title'] ?? null,
            isAdult: $apiData['adult'] ?? false,
            imdbID: $apiData['imdb_id'] ?? null,
            translations: Translation::arrayFromAPIData($apiData['translations'] ?? [])),
        );
    }
    
    protected static function detailsAPIUri(int $id): string
    {
        return Something::apiHost . "/movie/{$id}";
    }
    
    // could be defined in a trait or common superclass
    protected static function arrayFromAPIData(array $apiData)
    {
        return array_map($this::createFromAPIData(...), $apiData);
    }
    
    // could be defined in a shared CanFetchTranslations trait
    public function fetchTranslations(): array
    {
        if (isset($this->translations)) {
            return $this->translations;
        }
        
        $uri = static::detailsAPIUri($this->id) . '/translations';
        $apiData = Something::get($uri);
        
        return $this->translations = Translation::arrayFromAPIData($apiData['translations'] ?? []));
    }
}

neildaniels avatar Dec 17 '21 00:12 neildaniels