Auto import Nextcloud classes in rector runs

[come-nc]

Summary

When using rector, fully qualified names are often outputted by the rules, like \OCP\Server or \OCP\Http\Client\IClientService. To avoid that, one can use withImportNames() to tell rector to import names with use statements, but then it will import all names.

This is not always desirable, for instance \LDAP\Connection in user_ldap would be confusing because there is an \OCA\User_LDAP\Connection class as well.

So I added a custom "SkipVoter" which skips auto-import for all classes outside of OC/OCA/OCP.

It may still be a bit confusing for common names like IManager, but I think we can live with that? Or I can add other exceptions if needed. Note that rector is smart enough to not import two classes with the same name, but it will randomly import one of them and keep the fully qualified name for the other one.

I’m doing that so that when we apply Nextcloud rules set later we do not get fully qualified names everywhere but nice use statements instead.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Screenshots before/after for front-end changes
• [ ] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)

[come-nc]

In quite a few places it is nice to use the fqn to avoid mistakes.

Do you have examples in mind so that we can look if there is a pattern we could apply?

Then there is also the case when two classes have the same name where I would actually prefer that both use the fqn and not one fqn and one import.

That is quite hard to implement I think. The SkipVoter which avoid use conflict is based on the already imported use list: https://github.com/rectorphp/rector/blob/main/rules/CodingStyle/ClassNameImport/ClassNameImportSkipVoter/UsesClassNameImportSkipVoter.php Doing the same thing without even adding the first use would require collecting all shortnames from the file before deciding whether to import them or not.

[provokateurin]

Could we maybe limit this to a few classes like \OCP\Server and \OCP\Util for now? I don't know how many other cases we have where this happens a lot.

[come-nc]

Could we maybe limit this to a few classes like \OCP\Server and \OCP\Util for now? I don't know how many other cases we have where this happens a lot.

My problem is this rule set: https://github.com/nextcloud-libraries/rector/blob/8061985c44798bac1657ba5bc2d6efa5b10e9089/config/nextcloud-25/nextcloud-25-deprecations.php#L17 Without withImportNames it will use fully qualified name for all Server::get() calls and that’s ugly. I can try again a solution like https://github.com/nextcloud-libraries/rector/pull/11/files to instead make our rules add stuff to the use statements independantly of withImportNames. I failed before but I have a better understanding of rector internals now.

[come-nc]

So, the approach in https://github.com/nextcloud-libraries/rector/pull/11/files failed.

@provokateurin What do you think about using the strategy in this PR (auto import from OC/OCA/OCP), but with a manually maitained list of exempted class names? (The list would be something like Plugin,Manager,IManager,Exception). Can I try that and see what the result looks like on our apps?

[provokateurin]

Sounds good to me, excluding common duplicate names :+1:

[come-nc]

Sounds good to me, excluding common duplicate names 👍

Can you check at the current diff if the exclusion list and the result makes sense to you?

[come-nc]

@miaulalala @nickvergessen @tcitworld I fixed psalm baseline, only thing missing now is your reviews as code owners.