Refactor cache.php to better conform with PSR 2 #2689
Conversation
The method _useCache doesn't seem to be in use outside the core cache classes[0], thus no deprecation placeholder was created. [0]: https://codesearch.dokuwiki.org/search?project=dokuwiki&project=plugin&project=template&q=&defs=&refs=_useCache&path=&hist=&type=php
inc/Cache/Cache.php
Outdated
| * | ||
| * @return bool see useCache() | ||
| */ | ||
| public function makdeDefaultCacheDecision() |
There was a problem hiding this comment.
- This function was named
_useCache()before. I like and understand renaming it to a self-explaining name. - However, the first part of the name is a bit strange for me. Is it the verb 'make' with a typo? or is this a German verb? Please improve.
- Please rename all the occurrences of the old
_useCache.
There was a problem hiding this comment.
There is a disconnect with the comment and the method visbility too. One should really be changed to match the other.
There was a problem hiding this comment.
@Klap-in I really didn't see that extra "d" until you pointed it out, thanks :)
I also renamed that method in two comments that still referred to _useCache. Did you see the old comment name anywhere else?
There was a problem hiding this comment.
@Chris--S I adjusted the method comment. Do you feel they fit each other better now? If not, what would be your suggestion?
That method cannot be private or protected, since it has to be used by the event handler. Thus adjust the method comment to internal to better represent that it shouldn't be used by other classes.
These fields might still be accessed from the outside, so in order to not break backwards compatibility, this uses magic methods that emit deprecation errors.
DokuWiki has its own way to report deprecation notifications. Let's use it!
|
We could handle renaming the fields by using something similar to https://github.com/wikimedia/mediawiki/blob/master/includes/debug/DeprecationHelper.php |
inc/Cache/Cache.php
Outdated
| trigger_error( | ||
| '\dokuwiki\Cache\Cache::_event is deprecated since 2019-02-02. Use \dokuwiki\Cache\Cache::getEvent()', | ||
| E_USER_DEPRECATED | ||
| ); |
There was a problem hiding this comment.
I wonder if this error is useful. Don't these errors work like a non-catchable exception, ending code execution? Or do I remember this wrong? Deprecated accesses should continue to work.
There was a problem hiding this comment.
Nope. The code
trigger_error(
'\dokuwiki\Cache\Cache::_event is deprecated since 2019-02-02. Use \dokuwiki\Cache\Cache::getEvent()',
E_USER_DEPRECATED
);results only in the following output in my error log:
[Tue Feb 19 21:51:54.001036 2019] [php7:notice] [pid 487] [client 127.0.0.1:49030] PHP Deprecated: \\dokuwiki\\Cache\\Cache::_event is deprecated since 2019-02-02. Use \\dokuwiki\\Cache\\Cache::getEvent() in /h
ome/michael/public_html/dokuwiki/inc/Cache/Cache.php on line 61, referer: http://127.0.0.1/~michael/dokuwiki/doku.php?id=start
[Tue Feb 19 21:51:54.001069 2019] [php7:notice] [pid 487] [client 127.0.0.1:49030] PHP Stack trace:, referer: http://127.0.0.1/~michael/dokuwiki/doku.php?id=start
[Tue Feb 19 21:51:54.001084 2019] [php7:notice] [pid 487] [client 127.0.0.1:49030] PHP 1. {main}() /home/michael/public_html/dokuwiki/lib/exe/css.php:0, referer: http://127.0.0.1/~michael/dokuwiki/doku.php?id=
start
[Tue Feb 19 21:51:54.001091 2019] [php7:notice] [pid 487] [client 127.0.0.1:49030] PHP 2. css_out() /home/michael/public_html/dokuwiki/lib/exe/css.php:20, referer: http://127.0.0.1/~michael/dokuwiki/doku.php?i
d=start
[Tue Feb 19 21:51:54.001104 2019] [php7:notice] [pid 487] [client 127.0.0.1:49030] PHP 3. dokuwiki\\Cache\\Cache->__set($name = '_event', $value = 'CSS_CACHE_USE') /home/michael/public_html/dokuwiki/lib/exe/cs
s.php:115, referer: http://127.0.0.1/~michael/dokuwiki/doku.php?id=start
[Tue Feb 19 21:51:54.001131 2019] [php7:notice] [pid 487] [client 127.0.0.1:49030] PHP 4. trigger_error('\\\\dokuwiki\\\\Cache\\\\Cache::_event is deprecated since 2019-02-02. Use \\\\dokuwiki\\\\Cache\\\\Cach
e::getEvent()', 16384) /home/michael/public_html/dokuwiki/inc/Cache/Cache.php:61, referer: http://127.0.0.1/~michael/dokuwiki/doku.php?id=start
There was a problem hiding this comment.
What if you have display_errors on in your php.ini?
There was a problem hiding this comment.
I expect that you then see those error on your website (which you want in dev).
One shouldn't have display_erros on in production.
That being said, maybe this isn't such a good idea after all. As I still get these notices in css.php and js.php where those error are especially annoying.
I will try to have another look at it on Thursday or Friday evening,
Instead of writing our own magic getters and setters for all variables that used to be public, this adds a trait that does that in a generic way. This trait was copied from MediaWiki and adjusted to DokuWiki. The original author seems to be @tgr Tisza Gergő The downside of this trait is that the properties keep their (potentially undesired) name. While that could be fixed within the helper, that might add unnecessary complexity. The name can change when support is dropped.
|
I'm not yet happy with the output produced by or Both don't seem to be ideal. I'll have another look into this on Sunday. |
|
I think your second suggestion looks fine. |
That is also what is currently implemented. What I don't like about it is that it points to the line where the method is invoked that calls accesses/sets that property, not the line where that property is accessed/invoked. So, a combination of those two might be ideal. |
This gives us the flexibility to handle both deprecated properties and methods/functions properly.
Remove these errors pending proper discussion. These errors could be a problem for installations that have HTML errors on in production. Since DokuWikis is often used in smaller and private contexts, this is a scenario that should be considered.
|
@splitbrain I've created a new class to do the deprecation-logging properly and am now happy with this PR. Do you want to have another look? This also removes the last |
|
awesome work! thank you |
|
@Klap-in I'll gladly update the docs, but I'm not sure which need updating. Could you elaborate? |
|
In the debugger the event Further some wiki articles explain internals of the cache mechanism. And mention therefore internal functions like |
|
Ah, I see. I will have a look at it this evening or tomorrow. |
|
The event is now documented, though it might need an update when the PSR-2 PR is actually merged: https://www.dokuwiki.org/devel:event:info_deprecation_log Other pages that could use an update when the PSR-2 PR is merged: |
This is part of the PSR 2 refactoring effort in #2358 and should be merged into that branch.
This PR splits up
inc/cache.phpinto a file for every class and leaves thin wrappers behind that triggerE_USER_DEPRECATEDerrors.This also fixes the fields in the class which had still leading underscores to indicate their visibility. It adds magic getters and setters to maintain backwards compatibility. They should be removed after the next release or after the one after that.