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

Global function usage only in error with phpstan but not in production/unit test despite migration doc #979

Closed
1 of 3 tasks
liqueurdetoile opened this issue Nov 10, 2023 · 5 comments
Labels
Milestone

Comments

@liqueurdetoile
Copy link
Contributor

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Application Skeleton Version: 5.0.1

  • Platform and Target: PHP 8.2.11

Hey there,

Just starting with a fresh 5x app install, I was facing a phpstan error with the global translation function __ :

--------------------------------------------------------------------- 
  Line   Model\Table\WebsitesTable.php
 ------ ---------------------------------------------------------------------
  :96    Function __ not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

Curiously my unit tests were working fine and not complaining about global __ use. Wandering why, I've found out that my default app's config/bootstrap.php still has require CAKE . 'functions.php';.

I've read once (and forgot till now) in the migration guide that

Global functions are now opt-in. If your application uses global function aliases be sure to add require CAKE . 'functions.php' to you application’s config/bootstrap.php.

We're in the middle of the river with current setup 😬 Either, the default bootstrap should be updated (and my unit tests would have quick failed), either, default phpstan config should be updated :

parameters:
    level: 8
    treatPhpDocTypesAsCertain: false
    checkGenericClassInNonGenericObjectType: false
    # add this one
    bootstrapFiles:
        - vendor/cakephp/cakephp/src/functions.php
    paths:
        - src/

Finally, it's also possible to get rid of these globals as they are now defined in namespace. For migration ease, they can be split in an optional compatibility plugin so it won't lure IDE with unrequired globals if typing __( too quickly if plugin is not installed.

It's a really quick and naive approach and I'm certainly missing many elements to say what is the best option. If using an IDE, last one seems to be the best though.

By the way, you've made a hard work with CakePHP 5.x. Thanks !

@markstory markstory added this to the 5.x milestone Nov 10, 2023
@markstory
Copy link
Member

We're in the middle of the river with current setup 😬 Either, the default bootstrap should be updated (and my unit tests would have quick failed), either, default phpstan config should be updated :

Updating the phpstan configuration seems like the lowest friction change here. Removing functions.php from application startup is another viable option. I don't prefer that approach because we still have a lot of documentation and examples that are using global functions and I'd prefer to not create confusion for new folks.

For migration ease, they can be split in an optional compatibility plugin so it won't lure IDE with unrequired globals if typing __( too quickly if plugin is not installed.

This is a good option as well. Moving global functions out into a package seems like a good change to make for cake 6. We can't really move it for 5.x as that would break compatibility outside of a major release which is something we try to avoid doing.

@ADmad
Copy link
Member

ADmad commented Nov 10, 2023

We should update the phpstan config.

Even going forth I would prefer the global functions be opt-out (as they are now) instead of opt-in. The primary reason I namespaced all the functions was to avoid potential conflicts with other frameworks/libs (when using split packages for e.g), not to remove the convenience for CakePHP users.

@liqueurdetoile
Copy link
Contributor Author

@ADmad You're definitely right if keeping globals in core in order to be consistent with autocompletion and I also agree with @markstory that moving them to a package should require a major release.

@ADmad
Copy link
Member

ADmad commented Nov 10, 2023

@liqueurdetoile Can you please make a PR updating the phpstan config?

@liqueurdetoile
Copy link
Contributor Author

@ADmad Sure. Should I also propose a PR for docs ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants