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

More missing rectors #266

Open
dereuromark opened this issue Dec 12, 2023 · 9 comments
Open

More missing rectors #266

dereuromark opened this issue Dec 12, 2023 · 9 comments

Comments

@dereuromark
Copy link
Member

In test cases

protected $fixtures = [

should be

protected array $fixtures = [
@LordSimal
Copy link
Contributor

This would require separate ruleset and adjusted docs since it needs to be applied to the tests folder, not the src folder

@dereuromark
Copy link
Member Author

I dont think so.
I always run the upgrade over all folders, src and tests.
It will just ignore the ones that dont match.

@LordSimal
Copy link
Contributor

Cool that you do it but at least in our docs we reference that these rules should only be applied to the src directory.
https://book.cakephp.org/5/en/appendices/5-0-upgrade-guide.html

Also parsing unnecessary files in rector only increases the upgrade time so trying to apply rector rules in the tests folder which can never be applied seems like an obvious waste of ressources.

@dereuromark
Copy link
Member Author

You misread that statement IMO
The point is to use a folder inside ROOT, never ROOT
Doing ROOT will explode your computer, but using src/, tests/, config/ or any other non root (thus non vendor) folder works out of the box super quick and easy, no problems.

So we can also just clarify the docs here further :)

@LordSimal
Copy link
Contributor

LordSimal commented Dec 12, 2023

Then the docs are 100% not clear, agreed

@LordSimal
Copy link
Contributor

Could also be the fact that I never had something inside tests which would have automatically be fixed via the rector tool.... could also be how I write my tests... 😅

@markstory
Copy link
Member

Should we detect that the provided path is a project root and do the right thing for users? If we can find a src/, tests/, config/ directories we could add those to the list instead of the provided directory. Might make using the upgrade tool simpler to operate. I've bumped into this in the past as well.

@dereuromark
Copy link
Member Author

Also:

  • TableRegistry::get() could maybe be fixed up to the non deprecated version

@dereuromark
Copy link
Member Author

Also tried adding

new ModalToGetSet('Cake\ORM\Entity', 'source'),

into 3.4 set

but it didnt seem to work.
The source() method is on the trait.. maybe thats the issue.

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

No branches or pull requests

3 participants