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

Fix broken ReflectionUnionTypes #1132

Merged
merged 12 commits into from
Jan 11, 2021
Merged

Fix broken ReflectionUnionTypes #1132

merged 12 commits into from
Jan 11, 2021

Conversation

fabio-ivona
Copy link
Contributor

@fabio-ivona fabio-ivona commented Dec 21, 2020

Summary

This PR addresses issue #1129 , fixing exceptions thrown when a union type is present both in return type and parameter types

In php 8 ReflectionType class is subclassed by ReflectionUnionType , which is missing getName() and isBuiltin() methods. This code changes aim at defining an intermediary function that:

  • handles ReflectionUnionTypes by splitting them in an array of ReflectionNamedTypes.
  • handles single ReflectionNamedTypes, wrapping them in an array in order to handle the same data type
protected function extractReflectionTypes(\ReflectionType $reflection_type)
{
    if($reflection_type instanceof \ReflectionUnionType){
        foreach ($reflection_type->getTypes() as $named_type){
            if($named_type->getName()==='null'){
                continue;
            }

            $types[] = $this->getReflectionNamedType($named_type);
        }
    }else{
        $types[] = $this->getReflectionNamedType($reflection_type);
    }

    return $types;
}

the getReflectionNamedType() function wraps the old code for extracting the parameter name and adding \\ in case of builtin type:

protected function getReflectionNamedType(\ReflectionNamedType $paramType): string
{
    $parameterName = $paramType->getName();
    if (!$paramType->isBuiltin()) {
        $parameterName = '\\' . $parameterName;
    }

    return $parameterName;
}

These two functions are used to convert the two subclasses of ReflectionTypes to their respective dockblock strings

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md (no changes needed)
  • Code style has been fixed via composer fix-style

this commit fixes exceptions thrown on ReflectionUnionType::isBuiltIn() and ReflectionUnionType::getNme() called on php8 union types
this commit fixes exceptions thrown on ReflectionUnionType::isBuiltIn() and ReflectionUnionType::getNme() called on php8 union types
# Conflicts:
#	src/Console/ModelsCommand.php
@fabio-ivona fabio-ivona changed the title Fix broken ReflectionUnionTypes WIP: Fix broken ReflectionUnionTypes Dec 21, 2020
@mfn
Copy link
Collaborator

mfn commented Dec 21, 2020

@def-studio first quick look looks good! you mentioned in the issue assistance with tests => let me ask you first, did you check out the existing ones? Especially the ModelsCommand has lots of them, please see https://github.com/barryvdh/laravel-ide-helper/tree/master/tests/Console/ModelsCommand

@fabio-ivona
Copy link
Contributor Author

@def-studio first quick look looks good! you mentioned in the issue assistance with tests => let me ask you first, did you check out the existing ones? Especially the ModelsCommand has lots of them, please see https://github.com/barryvdh/laravel-ide-helper/tree/master/tests/Console/ModelsCommand

Just checking it, will update code soon (or ask for clarifications if needed 😇 )

@fabio-ivona
Copy link
Contributor Author

@mfn I've added a new test directory tests/Console/ModelsCommand/UnionTypes which tests a model with some use cases:

union types in parameters:

public function scopeWithUnionTypeParameter(Builder $query, string|int $bar): Builder
{
    return $query->where('foo', $bar);
}

nullable union type in parameters:

public function scopeWithNullableUnionTypeParameter(Builder $query, null|string|int $bar): Builder
{
    return $query->where('foo', $bar);
}

union type in return type:

public function withUnionTypeReturn(): HasMany|UnionTypeModel
{
    return $this->hasMany(UnionTypeModel::class);
}

nullable union type in return type:

public function getFooAttribute(): string|int|null
{
    return $this->getAttribute('foo');
}

@fabio-ivona fabio-ivona changed the title WIP: Fix broken ReflectionUnionTypes Fix broken ReflectionUnionTypes Dec 21, 2020
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😄

@barryvdh barryvdh merged commit 73c502d into barryvdh:master Jan 11, 2021
kodiakhq bot referenced this pull request in WesleyKlop/vote-system Mar 16, 2021
Bumps [barryvdh/laravel-ide-helper](https://github.com/barryvdh/laravel-ide-helper) from 2.9.0 to 2.9.1.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/barryvdh/laravel-ide-helper/releases">barryvdh/laravel-ide-helper's releases</a>.</em></p>
<blockquote>
<h2>v2.9.1</h2>
<h3>Added</h3>
<ul>
<li>Generate PHPDoc for Laravel 8.x factories <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1074">#1074 / ahmed-aliraqi</a></li>
<li>Add a comment to a property like table columns <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1168">#1168 / biiiiiigmonster</a></li>
<li>Added <code>post_migrate</code> hook to run commands after a migration <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1163">#1163 / netpol</a></li>
</ul>
<h3>Fixed</h3>
<ul>
<li>Error when generating helper for invokable classes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1124">#1124 / standaniels</a></li>
<li>Fix broken ReflectionUnionTypes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1132">#1132 / def-studio</a></li>
<li>Relative class names are not converted to fully-qualified class names <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1005">#1005 / SavKS</a></li>
</ul>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/barryvdh/laravel-ide-helper/blob/master/CHANGELOG.md">barryvdh/laravel-ide-helper's changelog</a>.</em></p>
<blockquote>
<h2>2021-03-15, 2.9.1</h2>
<h3>Added</h3>
<ul>
<li>Generate PHPDoc for Laravel 8.x factories <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1074">#1074 / ahmed-aliraqi</a></li>
<li>Add a comment to a property like table columns <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1168">#1168 / biiiiiigmonster</a></li>
<li>Added <code>post_migrate</code> hook to run commands after a migration <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1163">#1163 / netpol</a></li>
</ul>
<h3>Fixed</h3>
<ul>
<li>Error when generating helper for invokable classes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1124">#1124 / standaniels</a></li>
<li>Fix broken ReflectionUnionTypes <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1132">#1132 / def-studio</a></li>
<li>Relative class names are not converted to fully-qualified class names <a href="https://github.com/barryvdh/laravel-ide-helper/pull/1005">#1005 / SavKS</a></li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/8d8302ff6adb55f8b844c798b8b1ffdee142f7e5"><code>8d8302f</code></a> chore: add missing changelog entries (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1176">#1176</a>)</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/202395b50f31325911fd045993c2b4d2851b8bf6"><code>202395b</code></a> Auto generate models (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1163">#1163</a>)</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/c5c9b2b9517696e1d17ce64790be42b8bb76a4d2"><code>c5c9b2b</code></a> Add comments like table columns. (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1168">#1168</a>)</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/c5e18beff1a2933913f86d56281581566e5c8705"><code>c5e18be</code></a> Fix Relative class names are not converted to fully-qualified class names (FQ...</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/6c4dcd9049e7e9aabc78673de7d1b9d428f3dc10"><code>6c4dcd9</code></a> Merge pull request <a href="https://github.com/barryvdh/laravel-ide-helper/issues/1164">#1164</a> from wilsenhc/readme-fix</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/aaade4eab932efe3841306ad7faf9ef1a8cef391"><code>aaade4e</code></a> Fix typo in README</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/b21324b20e24a3c36bedc184b4cf273bfa0a2d32"><code>b21324b</code></a> Merge pull request <a href="https://github.com/barryvdh/laravel-ide-helper/issues/1156">#1156</a> from ZaidBarghouthi/fix-zb-extra-quote</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/db83fb8b6ba38ea0b1d28197e898b4ff0d9581d0"><code>db83fb8</code></a> Remove extra single quote</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/4f42af1fa1f6265174ef97c36e0ae7beb2596c29"><code>4f42af1</code></a> Apply &quot;<a href="https://github.com/PSR12"><code>@​PSR12</code></a>&quot; in php-cs-fixer (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1150">#1150</a>)</li>
<li><a href="https://github.com/barryvdh/laravel-ide-helper/commit/f0959c1184c6f8f9e50afa6d9b6c1eb7691ae3ae"><code>f0959c1</code></a> Update changelog and also automagically apply style fix (<a href="https://github.com/barryvdh/laravel-ide-helper/issues/1140">#1140</a>)</li>
<li>Additional commits viewable in <a href="https://github.com/barryvdh/laravel-ide-helper/compare/v2.9.0...v2.9.1">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=barryvdh/laravel-ide-helper&package-manager=composer&previous-version=2.9.0&new-version=2.9.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>
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.

3 participants