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

Turn on logging for deprecation notices by default #6773

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

paulbalandan
Copy link
Member

Description
Following up on #6705 , this turns on the feature by default.

Deprecation notices should not halt execution. These notices are here to notify the users that something will not work in the future. They should not be forced to fix it now.

This PR changes the following:

  • Turns on deprecation logging
  • Raise logger reporting threshold to 5
  • Use the env variable CODEIGNITER_SCREAM_DEPRECATIONS to temporarily throw deprecations in tests (for convenience rather than changing the config).

Related: #6764

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis
Copy link
Member

kenjis commented Oct 28, 2022

Why does this error occur?

1) CodeIgniter\Cookie\CookieStoreTest::testCookieDispatching
ErrorException: The CodeIgniter\Cookie\CookieStore::setCookie method is deprecated ().

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Cookie/CookieStore.php:174
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cookie/CookieStoreTest.php:119
/home/runner/work/CodeIgniter4/CodeIgniter4/vendor/nexusphp/tachycardia/src/Expeditable.php:43
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:97

https://github.com/codeigniter4/CodeIgniter4/actions/runs/3343093343/jobs/5535960965

system/Debug/Exceptions.php Outdated Show resolved Hide resolved
app/Config/Exceptions.php Outdated Show resolved Hide resolved
app/Config/Logger.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Oct 28, 2022

Why does this error occur?

I'm clueless! Maybe PHPUnit is trigger a deprecation warning? We know it parses docblock tags.

@MGatner
Copy link
Member

MGatner commented Oct 28, 2022

I didn't know this feature existed: https://phpunit.readthedocs.io/en/9.5/configuration.html#the-ignoredeprecatedcodeunits-attribute

Doesn't explicitly seem like it would actually trigger a deprecation though.

@paulbalandan
Copy link
Member Author

Why does this error occur?

Apparently, PHPUnit parses @deprecated tags when mocking a method:
https://github.com/sebastianbergmann/phpunit/blob/851867efcbb6a1b992ec515c71cdcf20d895e9d2/src/Framework/MockObject/MockMethod.php#L123-L130

When it detects the tag, it generates the @trigger_error() in a template:
https://github.com/sebastianbergmann/phpunit/blob/851867efcbb6a1b992ec515c71cdcf20d895e9d2/src/Framework/MockObject/MockMethod.php#L205-L216

@MGatner
Copy link
Member

MGatner commented Oct 30, 2022

I think this one has to get into 4.3 because of #6705. Is that ground for pushing on #6710 as well? When remains to be done here?

@paulbalandan
Copy link
Member Author

I think this one has to get into 4.3 because of #6705. Is that ground for pushing on #6710 as well? When remains to be done here?

I'm not sure what justification do you want for the increase in logged level. That's what remains here.

@kenjis
Copy link
Member

kenjis commented Oct 30, 2022

I don't think $threshold should be changed easily.

@paulbalandan paulbalandan force-pushed the deprecation branch 6 times, most recently from d8fd5e6 to 43fce71 Compare October 30, 2022 16:18
@MGatner
Copy link
Member

MGatner commented Oct 31, 2022

How about some way of supporting different environment-specific levels? Like if $threshold = ENVIRONMENT === 'production' ? 4 : 9

This won't affect existing installations nor current logging expectations in production, and developers can always change it in new projects.

@kenjis
Copy link
Member

kenjis commented Oct 31, 2022

It seems good! I always change it to 9 in development.

-    public $threshold = 4;
+    public $threshold = (ENVIRONMENT === 'production') ? 4 : 9;

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks team! Very nice work @paulbalandan, glad to have this one in.

@paulbalandan paulbalandan merged commit 2ce4b48 into codeigniter4:4.3 Nov 1, 2022
@paulbalandan paulbalandan deleted the deprecation branch November 1, 2022 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants