Add better support for macros and mixins #1006

domkrm · 2020-08-08T19:44:13Z

I have added macro support for all macroable classes, not only the alias classes.
But this only works if the macros are declared in any loaded service provider and type hinting is used.
Also no description is possible, only parameter and return types.

mfn

Hey 👋 very cool approach!

I left some in-line feedback.

Also: All / some? of the whitespace changes in resources/views/helper.php generate a vastly differently indented _ide_helper.php file:

run master, generate _ide_helper.php , make a copy
run your PR, generate _ide_helper.php
diff them
💥

People tend to commit this file and those two versions are essentially "uncomparable".

I don't want to say it's the most important thing in the world, but some of those changes just look unnecessary anyway, so you might want to massage this a bit.

Maybe also rebase against master, there were some formatting changes already, creating merge conflicts currently.

Seeing that I had some exceptions thrown my way, we definitely should come up with some tests for this.

README.md

mfn · 2020-08-20T20:05:00Z

src/Macro.php

+
+        // Add macro parameters
+        foreach ($method->getParameters() as $parameter) {
+            $type = $parameter->hasType() ? $parameter->getType() : 'mixed';


./artisan ide-helper:generate ErrorException Function ReflectionType::__toString() is deprecated at vendor/barryvdh/laravel-ide-helper/src/Macro.php:45 41| 42| // Add macro parameters 43| foreach ($method->getParameters() as $parameter) { 44| $type = $parameter->hasType() ? $parameter->getType() : 'mixed'; > 45| $type .= $parameter->hasType() && $parameter->getType()->allowsNull() ? '|null' : ''; 46| 47| $name = $parameter->isVariadic() ? '...' : ''; 48| $name .= '$' . $parameter->getName(); 49| +21 vendor frames 22 artisan:37 Illuminate\Foundation\Console\Kernel::handle()

I guess this is dependent on the PHP version? It's "just" a deprecation but people might run with E_ALL and thus trip over this.

This fixes it:

Suggested change

$type = $parameter->hasType() ? $parameter->getType() : 'mixed';

$type = $parameter->hasType() ? $parameter->getType()->getName() : 'mixed';

I have changed this to your suggestion.

mfn · 2020-08-20T20:05:54Z

src/Macro.php

+
+        // Add macro return type
+        if ($method->hasReturnType()) {
+            $type = $method->getReturnType();


Here I have the same issue:

$ ./artisan ide-helper:generate ErrorException Function ReflectionType::__toString() is deprecated at vendor/barryvdh/laravel-ide-helper/src/Macro.php:56 52| 53| // Add macro return type 54| if ($method->hasReturnType()) { 55| $type = $method->getReturnType(); > 56| $type .= $method->getReturnType()->allowsNull() ? '|null' : ''; 57| 58| $this->phpdoc->appendTag(Tag::createInstance("@return {$type}")); 59| } 60| } +21 vendor frames 22 artisan:37 Illuminate\Foundation\Console\Kernel::handle()

Suggested change

$type = $method->getReturnType();

$type = $method->getReturnType()->getName();

I have changed this to your suggestion.

domkrm · 2020-08-23T17:16:47Z

I have adopted all of your suggestions. But I also have changed many things because I noticed that classes could be duplicate in the helper file if they have macros (Facades, Arr, Str, ...).

mfn

Approach LGTM and I only have some minor feedback; additionally:

can you please rebase on master and solve the merge conflict?
re-add the nice README entry you had?
add a changelog entry?
Yes, we now have a changelog 😄

Thanks and sorry for the long delay, rest assured unless I get hit by a bus, the next feedback will be quicker 🤞

mfn · 2020-09-05T20:19:01Z

src/Generator.php

+     */
+    protected function getMacroableClasses(Collection $aliases)
+    {
+        return collect(get_declared_classes())


collect

Please don't use global helpers, it's a bad practice doing this in libraries. None of the helpers from Illuminate\Support\helpers.php is currently in use.

Using the collection of course is fine!

mfn · 2020-09-05T20:19:20Z

src/Generator.php

+                $traits = class_uses($class);
+
+                // Filter only classes with the macroable trait
+                return isset($traits['Illuminate\Support\Traits\Macroable']);


Please use Illuminate\Support\Traits\Macroable::class and also import it, thanks!

mfn · 2020-09-05T20:23:30Z

src/Macro.php

+            $type = $parameter->hasType() ? $parameter->getType()->getName() : 'mixed';
+            $type .= $parameter->hasType() && $parameter->getType()->allowsNull() ? '|null' : '';
+
+            $name = $parameter->isVariadic() ? '...' : '';
+            $name .= '$' . $parameter->getName();


Looks very clean but also duplicate code of what happens in \Barryvdh\LaravelIdeHelper\Method::getParameters

the other method seems more feature-complete though I can't say if this is necessary

but it's written in a way which makes it not easily re-usable AFAICS, i.e. you just can't call it.

Would you be interested comparing the the two approaches and see if yours is missing something? E.g. default values seems to be handled, etc.

Unifying this is topic for a different PR IMHO and shouldn't be done here.

mfn · 2020-09-05T20:24:35Z

src/Generator.php

+                $reflection = new ReflectionClass($class);
+
+                // Filter out internal classes and class aliases
+                return !$reflection->isInternal() && $reflection->getName() == $class;


Suggested change

return !$reflection->isInternal() && $reflection->getName() == $class;

return !$reflection->isInternal() && $reflection->getName() === $class;

mfn · 2020-09-05T20:24:43Z

src/Generator.php

+
+                // Filter out aliases
+                return !$aliases->first(function (Alias $alias) use ($class) {
+                    return $alias->getExtends() == $class;


Suggested change

return $alias->getExtends() == $class;

return $alias->getExtends() === $class;

mfn · 2020-09-05T20:32:51Z

Btw, do we still need the code in \Barryvdh\LaravelIdeHelper\Alias::detectMethods 🤔 What's the difference?

            // Check if the class is macroable
            $traits = collect($reflection->getTraitNames());
            if ($traits->contains('Illuminate\Support\Traits\Macroable')) {
                $properties = $reflection->getStaticProperties();
                $macros = isset($properties['macros']) ? $properties['macros'] : [];
                foreach ($macros as $macro_name => $macro_func) {
                    if (!in_array($macro_name, $this->usedMethods)) {
                        // Add macros
                        $this->methods[] = new Macro(
                            $this->getMacroFunction($macro_func),
                            $this->alias,
                            $reflection,
                            $macro_name,
                            $this->interfaces
                        );
                        $this->usedMethods[] = $macro_name;
                    }
                }
            }

domkrm · 2020-09-05T21:30:28Z

I have done a rebase and applied all your suggestions.

domkrm · 2020-09-05T21:31:44Z

Btw, do we still need the code in \Barryvdh\LaravelIdeHelper\Alias::detectMethods 🤔 What's the difference?

            // Check if the class is macroable
            $traits = collect($reflection->getTraitNames());
            if ($traits->contains('Illuminate\Support\Traits\Macroable')) {
                $properties = $reflection->getStaticProperties();
                $macros = isset($properties['macros']) ? $properties['macros'] : [];
                foreach ($macros as $macro_name => $macro_func) {
                    if (!in_array($macro_name, $this->usedMethods)) {
                        // Add macros
                        $this->methods[] = new Macro(
                            $this->getMacroFunction($macro_func),
                            $this->alias,
                            $reflection,
                            $macro_name,
                            $this->interfaces
                        );
                        $this->usedMethods[] = $macro_name;
                    }
                }
            }

Yes, this code is still needed. My old approach had an own function to detect macros but my new approach uses this part of the code.

mfn

Thanks, IMHO this looks great \o/

@barryvdh IMHO we're good to merge here!

Co-authored-by: Dominik Krämer <[email protected]>

mfn requested changes Aug 20, 2020

View reviewed changes

mfn requested changes Sep 5, 2020

View reviewed changes

Add better support for macros and mixins

a336e33

mfn approved these changes Sep 5, 2020

View reviewed changes

barryvdh merged commit 3a22b8d into barryvdh:master Sep 6, 2020

mfn mentioned this pull request Sep 10, 2020

Validate method not found in Illuminate\Http\Request #608

Closed

mfn mentioned this pull request Sep 21, 2020

Generate Command throws on Http Macros #1065

Closed

hackel mentioned this pull request Sep 25, 2020

Support Eloquent Query Builder Macros #1070

Closed

LastDragon-ru mentioned this pull request Nov 28, 2020

Macro docblock regression: empty docblock #1099

Closed

LastDragon-ru mentioned this pull request Dec 6, 2020

Macro::initPhpDoc() will save original docblock if present. #1116

Merged

2 tasks

dmitry-ivanov mentioned this pull request Mar 16, 2021

Macros in custom locations? #1181

Closed

tarik02 pushed a commit to tarik02/laravel-ide-helper that referenced this pull request Jul 15, 2021

Add better support for macros and mixins (barryvdh#1006)

19a36a2

Co-authored-by: Dominik Krämer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better support for macros and mixins #1006

Add better support for macros and mixins #1006

domkrm commented Aug 8, 2020 •

edited

Loading

mfn left a comment

mfn Aug 20, 2020

domkrm Aug 23, 2020

mfn Aug 20, 2020

domkrm Aug 23, 2020

domkrm commented Aug 23, 2020

mfn left a comment

mfn Sep 5, 2020

mfn Sep 5, 2020

mfn Sep 5, 2020

mfn Sep 5, 2020

mfn Sep 5, 2020

mfn commented Sep 5, 2020

domkrm commented Sep 5, 2020

domkrm commented Sep 5, 2020

mfn left a comment

	$type = $parameter->hasType() ? $parameter->getType() : 'mixed';
	$type = $parameter->hasType() ? $parameter->getType()->getName() : 'mixed';

	$type = $method->getReturnType();
	$type = $method->getReturnType()->getName();

	return !$reflection->isInternal() && $reflection->getName() == $class;
	return !$reflection->isInternal() && $reflection->getName() === $class;

	return $alias->getExtends() == $class;
	return $alias->getExtends() === $class;

Add better support for macros and mixins #1006

Add better support for macros and mixins #1006

Conversation

domkrm commented Aug 8, 2020 • edited Loading

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domkrm commented Aug 23, 2020

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfn commented Sep 5, 2020

domkrm commented Sep 5, 2020

domkrm commented Sep 5, 2020

mfn left a comment

Choose a reason for hiding this comment

domkrm commented Aug 8, 2020 •

edited

Loading