Skip to content

[alternative] Use NodeVisitor for init attribute flag over global Node traverse#444

Merged
calebdw merged 5 commits intodriftingly:mainfrom
samsonasik:use-node-visitor
Jan 20, 2026
Merged

[alternative] Use NodeVisitor for init attribute flag over global Node traverse#444
calebdw merged 5 commits intodriftingly:mainfrom
samsonasik:use-node-visitor

Conversation

@samsonasik
Copy link
Copy Markdown
Collaborator

@samsonasik samsonasik commented Dec 10, 2025 •
edited
Loading

@peterfox per your feedback at #443 (comment)

This move to use NodeVisitor over global Node. Here the benefit and drawback:

Benefit:

  • can reuse class constant attribute over rules if needed.

Drawback:

  • The default config is included in sets, but if user just want to use some rule that require the attribute, user need to import the default config:
->withSets([
     __DIR__ . '/vendor/driftingly/rector-laravel/config/config.php'
])
->withRules([
     // the rule from this repo :)
]);

I've updated readme for it tho.

@samsonasik
Copy link
Copy Markdown
Collaborator Author

samsonasik commented Dec 10, 2025

@GeniJaho @driftingly @peterfox ready for review/merge 👍

@calebdw
Copy link
Copy Markdown
Collaborator

calebdw commented Jan 19, 2026

@samsonasik, is there any way for this package to automatically register this config so users don't have to when using single rules?

@samsonasik
Copy link
Copy Markdown
Collaborator Author

samsonasik commented Jan 19, 2026 •
edited
Loading

@calebdw I am on mobile, not sure, there is rector-extension-installer, but currently only used under rector organization that require-dev of rector-src include usage and require php 8

https://github.com/rectorphp/extension-installer/tree/main?tab=readme-ov-file#instructions-for-extension-developers

@calebdw
Copy link
Copy Markdown
Collaborator

calebdw commented Jan 20, 2026

Ah, looks handy! Do you think we could use it as part of this PR?

@samsonasik
Copy link
Copy Markdown
Collaborator Author

samsonasik commented Jan 20, 2026

I am not sure as that require php 8.0, and this repo require php 7.4

rector-laravel/composer.json

Line 7 in aee9d4a

"php": "^7.4 || ^8.0",

On rector, the generated config updated on build, while on this, this means that needs to be part of "require"

@calebdw
Copy link
Copy Markdown
Collaborator

calebdw commented Jan 20, 2026

I don't think this repo needs to require the rector-extension-installer, we just have to include the rector key in the composer.json

For example, phpstan packages don't need to require the phpstan-extension-installer, just include the config in the composer.json

@samsonasik
Copy link
Copy Markdown
Collaborator Author

samsonasik commented Jan 20, 2026

I currently don't have much energy (less sleep due to PHPStan 2.1.34 yesterday), could you handle that ? Thank you.

@calebdw calebdw merged commit a8408c7 into driftingly:main Jan 20, 2026
5 checks passed
@samsonasik samsonasik deleted the use-node-visitor branch January 20, 2026 01:47
@calebdw calebdw mentioned this pull request Jan 20, 2026
Merged
calebdw added a commit to calebdw/rector-laravel that referenced this pull request Jan 20, 2026
@calebdw
Revert "[alternative] Use NodeVisitor for init attribute flag over gl…
ba02a7c 
…obal Node traverse (driftingly#444)"

This reverts commit a8408c7.
@calebdw calebdw mentioned this pull request Jan 20, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peterfox peterfox Awaiting requested review from peterfox

@GeniJaho GeniJaho Awaiting requested review from GeniJaho

@driftingly driftingly Awaiting requested review from driftingly

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@samsonasik @calebdw