Conversation
|
CI shows in https://drone.owncloud.com/owncloud/core/7982/69 errors regarding autoloading in migrations need some closer view if we skip that in phpstan - include it otherwise Also autoloading related to Alternative approach would be - to point to every directory relevant for autoloading directly ( but from current experimentation I find it quite knowledgeable to see if our autoloader works properly ) |
Codecov Report
@@ Coverage Diff @@
## master #31810 +/- ##
============================================
+ Coverage 64% 64.03% +0.02%
+ Complexity 18190 18187 -3
============================================
Files 1178 1178
Lines 68726 68697 -29
Branches 1271 1271
============================================
- Hits 43990 43987 -3
+ Misses 24366 24340 -26
Partials 370 370
Continue to review full report at Codecov.
|
|
Note for when this gets to the "backport to stable10" stage: |
migrations are never loaded using the autoloader but purly on basis of requiring the explicit file. |
|
Hi, author of PHPStan here 😊
Unfortunately, these kinds of errors are not permitted to be ignored (it led developers to think their configuration is fine, but it isn't), basically I won't let you shoot yourself in the foot 😊 Instead, you should exclude the directory from the analysis (or autoload those files in |
4396395 to
9602944
Compare
|
After merging Need to schedule some more time to move this forward. @ondrejmirtes Also do you recommend for a large project like this, to use our (legacy) application bootstrapping file - or use autoloading capabilities for stan. (On a sidenote - absolutely love phpstan 😍 ) |
|
@patrickjahns One option I forgot to tell you about - yes, you can use Thank you for your kind words 😊 |
|
You have several options for autoloading and they are equal:
|
|
@DeepDiver1975 @PVince81 - are we still using jcrop in the frontend? It ships with some demo php code - which is not required and leads to static code analyzer failures i.e. We can ignore the |
d947ea5 to
b05e8c0
Compare
|
@DeepDiver1975 @PVince81 |
|
@patrickjahns probaby because the app framework provides a DB layer based on entities which can be convenient in some cases. At some point there was the thought of using DB entities everywhere but we never got to plan porting everything over. |
|
That just seems like a mess all over again - I am happy to introduce static code analysis all over the place to uncover and prevent further things ;-) |
4be6974 to
3af5fcd
Compare
|
These errors are fixed in phpstan master branch |
3af5fcd to
85455b3
Compare
|
Note: this might be reasonable to do in |
|
backport |
Description
Introduce https://github.com/phpstan/phpstan into owncloud/core
The
phpstan.neonconfiguration is a very minimal configuration to get it running and prevent some of the problems/technical debt related totemplates/routes.phpetc.We currently use level 0 - which is the most lenient level - in this level, we already have ~ 400 errors/violations which need to be gradually adressed.
Some of them are related to Migrations and autoloading
General noticed the issue with autoloading - right now our internal autoloader requires ownCloud to be properly installed (and apps enabled) to be able to provide the classes for phpstan.
I would consider this also quite some technical debt which at some point would need some ❤️ . If possible at some point I see it feasible to move towards composer as being the only autoloader, and our class being registered to composer as authoritative autoloader.
Motivation and Context
How Has This Been Tested?
🤖
Types of changes
Checklist: