Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Typeables #51302

Closed
wants to merge 4 commits into from
Closed

[11.x] Typeables #51302

wants to merge 4 commits into from

Conversation

bert-w
Copy link
Contributor

@bert-w bert-w commented May 5, 2024

Typeables

There currently are two classes in the framework which offer the ability to retrieve a mixed value by a specific type, which are:

// Request objects (\Illuminate\Http\Request):
request()->string('userid'); // '1'
request()->boolean('is_admin'); // true
request()->integer('userid'); // 1
request()->float('price'); // 1.05

// Configuration objects (\Illuminate\Config\Repository):
config()->string('somekey.name'); // 'myname'
config()->boolean('somekey.enabled'); // false
config()->integer('somekey.port'); // 3010
config()->float('somekey.percentage') // 0.5
config()->array('somekey.percentage') // [0.5]

This PR proposes a new Typeable class (which casts types) and StrongTypeable class (which checks types and throws exceptions if it doesnt match) which serves as a centralized API through which developers can retrieve typed values for the following objects:

Code Value Type Is New
\Illuminate\Http\Request
request()->string('userid'); '1' Typeable
request()->boolean('is_admin'); true Typeable
request()->integer('userid'); 1 Typeable
request()->float('price'); 1.05 Typeable
request()->array('userid'); ['1'] Typeable ✔️
request()->date('mydate'); Carbon custom
request()->enum('myenum'); Enum custom
request()->collect('myarray'); Collection custom
\Illuminate\Config\Repository
config()->string('somekey.name'); 'myname' StrongTypeable
config()->boolean('somekey.enabled'); false StrongTypeable
config()->integer('somekey.port'); 3010 StrongTypeable
config()->float('somekey.percentage'); 0.5 StrongTypeable
config()->array('somekey.percentage'); [0.5] StrongTypeable
\Illuminate\Routing\Route
request()->route()->typed()->parameter->string('userid'); '1' Typeable ✔️
request()->route()->typed()->parameter->boolean('userid'); true Typeable ✔️
request()->route()->typed()->parameter->integer('userid'); 1 Typeable ✔️
request()->route()->typed()->parameter->float('userid'); 1.0 Typeable ✔️
request()->route()->typed()->parameter->array('userid'); ['1'] Typeable ✔️
\Illuminate\Console\Command
$command->typed()->argument->string('myargument'); '1' Typeable ✔️
$command->typed()->argument->boolean('myargument'); true Typeable ✔️
$command->typed()->argument->integer('myargument'); 1 Typeable ✔️
$command->typed()->argument->float('myargument'); 1.05 Typeable ✔️
$command->typed()->argument->array('myargument'); ['1'] Typeable ✔️
$command->typed()->option->string('myoption'); '1' Typeable ✔️
$command->typed()->option->boolean('myoption'); true Typeable ✔️
$command->typed()->option->integer('myoption'); 1 Typeable ✔️
$command->typed()->option->float('myoption'); 1.05 Typeable ✔️
$command->typed()->option->array('myoption'); ['1'] Typeable ✔️

The existing type helpers for request() and config() have not changed but instead refactored to use the Typeable/StrongTypeable code.

Copy link

github-actions bot commented May 5, 2024

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@antonkomarev
Copy link
Contributor

@bert-w what about nullable values, like nullableString?

@NickSdot
Copy link
Contributor

NickSdot commented May 6, 2024

@bert-w I like the idea to have this unified in the framework!

Did you consider making this a trait?

// incomplete just to give an idea

trait InteractsWithTypes
{
    public function typed(): object
    {
        $typed = [];

        foreach ($this->defineTypedProperties() as $key => $value) {

            // handle numeric/non numeric, and maybe allow a closure?
            $property = is_numeric($key) 
                     ? $value 
                     : $key;

            $typed[$property] = new Typeable();
        }

        return (object)$typed;
    }

    abstract function defineTypedProperties(): array;

    // optional: proxy methods for all types, but then new B/C implications. Perhaps a second, optional trait would do better.
}

And then wherever we want to use it, instead of introducing properties:

class SomeClass
{
    use InteractsWithTypes;
    
    protected function defineTypedProperties(): array
    {
        return [
            'argument', // numeric
            'option' => '' // non numeric, enables something more advanced? 
        ];
    }
}

Alternatively reflection could be used. But that's less explicit and probably a bit expensive in some cases?

We would end up with a slightly different api, though. But first, less chance to run into B/C implications. Second, less "pollution" of the class where used.

// usage

$this->typed()->argument->string('userid'); // '1'

@bert-w
Copy link
Contributor Author

bert-w commented May 6, 2024

@antonkomarev good question, I dont know where you would stop implementing these variants of mixed types (nullableString, nullableInteger, stringInteger?) For now it only supports single basic types. So if you have other requirements i guess you still have to result to manual annotations like so:

/** @var ?string $someValue */
$someValue = $request->get('some-value');

// Happy phpstan

@NickSdot I like the $this->typed()->argument->string('userid'); // '1' syntax although it is quite verbose. It would also solve the breaking changes I have currently.

@NickSdot
Copy link
Contributor

NickSdot commented May 6, 2024

A part of #51297 also attempts to make type methods available, and copies plenty of code over into the class in question. I think to centralise the api as proposed by bert-w makes sense. Otherwise, things seem to get a bit out of control @ duplication.

@bert-w another quick thought, do we really need two classes? Or would this do?

Typeable::string();
Typeable::stringStrict();
Typeable::integer();
Typeable::integerStrict();

@bert-w bert-w marked this pull request as ready for review May 8, 2024 19:17
@taylorotwell
Copy link
Member

I'm not sure I see a big developer benefit here atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants