Skip to content

Allow adding custom Macroable classes. #1629

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

Merged
merged 5 commits into from
Jan 1, 2025
Merged

Conversation

mathieutu
Copy link
Contributor

Summary

Some packages, like Filament, implements their own version of the Macroable trait.
Spatie even made their own version.

This PR allow adding any macro class we want to detect in the config file.

Thanks,
Mathieu.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Update the README.md
  • Code style has been fixed via composer fix-style

@mathieutu mathieutu marked this pull request as draft December 11, 2024 11:05
@mathieutu
Copy link
Contributor Author

mathieutu commented Dec 11, 2024

⚠️ This PR can't be merged as it, because of these lines:

$traits = collect($reflection->getTraitNames());
if ($traits->contains('Illuminate\Support\Traits\Macroable') || $class === EloquentBuilder::class) {

There are two problems with them:

  • We're not looking for traits recursively, contrary to what is done in the Generator. (Leads to bugs)
  • The Macroable class is hardcoded. (Topic of this PR)
  • (The code is dupplicated)

I'm ok to continue and make the changes.

However, could you tell me if you're interested in this or not?
If this PR will be closed anyway, I don't invest more time in it.

Thanks!

@barryvdh
Copy link
Owner

barryvdh commented Jan 1, 2025

I think this is okay to add. I would prefer to have the default macro class hardcoded, only add custom classes to the config.

@barryvdh barryvdh marked this pull request as ready for review January 1, 2025 12:15
@barryvdh barryvdh merged commit 05d9c3c into barryvdh:master Jan 1, 2025
17 checks passed
@mathieutu
Copy link
Contributor Author

mathieutu commented Jan 16, 2025

Hey @barryvdh Thanks for your help and the merge.
FYI, it's still not working with Filament, as I just saw they don't use the same macroable code as Laravel:
Laravel:
Screenshot 2025-01-16 at 17 22 08

Filament:
Screenshot 2025-01-16 at 17 21 59

Hence, it's obviously not compatible with this package Alias::getMacroFunction function... 😞

Not sure whether you'd like to provide a special treatment for them?

@erikn69
Copy link
Contributor

erikn69 commented May 13, 2025

it's still not working with Filament, as I just saw they don't use the same macroable code as Laravel

this break a lot, after regenerating my file (I don't do it often), most of my macros stopped working.

@erikn69 erikn69 mentioned this pull request May 13, 2025
8 tasks
erikn69 added a commit to erikn69/laravel-ide-helper that referenced this pull request May 14, 2025
erikn69 added a commit to erikn69/laravel-ide-helper that referenced this pull request May 14, 2025
erikn69 added a commit to erikn69/laravel-ide-helper that referenced this pull request May 14, 2025
barryvdh pushed a commit that referenced this pull request May 14, 2025
* Revert #1629 - Allow adding custom Macroable classes

* Add Macros test
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