[#35705] - [4.1] Remove useless calls to Cache::contains()
- Closed
- 21 Oct 2022
- Medium
- Build: 4.0-dev
- # 35705
- Diff
- Denitz:4.0_remove_callls_to_Cache__contains()
User tests: Successful: Unsuccessful:
Summary of Changes
The code performs useless calls to Cache::contains() before Cache::get() while it's enough to have Cache::get() only.
Cache::get() performs the cache key existence check and expiration itself.
Testing Instructions
Enable Joomla caching, apply patch.
Actual result BEFORE applying this Pull Request
Extra calls to cache storage contains() method.
Expected result AFTER applying this Pull Request
No extra calls to cache storage contains() method, page load time is smaller (minimal and almost impossible to detect).
Documentation Changes Required
No.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_finder Libraries Modules Front End |
| Title |
|
||||||
| Labels |
Added:
?
|
||
This pull requests has been automatically converted to the PSR-12 coding standard.
| Labels |
Added:
?
|
||
| Labels |
Added:
?
|
||
| Labels |
Removed:
?
|
||
Thank you for this PR. While this might be an optimised version, it does not comply with our coding standards and would be harder to maintain for us. I would be okay with accepting a slightly degraded performance for having this be more maintainable. So I'm going to close this PR.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-10-21 15:26:29 |
| Closed_By | ⇒ | Hackwar |
@Hackwar Please advise where I can read the coding standards about using of Cache:: contains()?
Please provide exact explanation why it will be harder to maintain and what?
Currently, the code looks like if (file_exists($path) && file_exists($path)). Besides, all other core cache calls are not using extra Cache:: contains().
Maybe we can ask few more developers? I would be happy to get help from @brianteeman and @nikosdion
Sorry, but I am not happy to introduce a normal PR into community-driven software, wait for more than a year and finally, receive the reject from one person without a clear reason.
It is indeed not good that you had to wait a year for feedback. We are working on improving that and if you take a look at the pull requests, we are now at least at a point where you "only" have to wait a year at max until your PR is processed. We had a time where we had PRs which were 5 years old...
My main concern with your code is the assignment inside of the if-statement. It might be totally fine code, but it is a source of confusion to figure out the order of precedence of the equal signs. It is simply something that we don't want to do in our code.
@Denitz Sorry for not replying earlier, off-line life got in the way.
Move the assignment outside (before) the if-block and leave a blank line between the assignment and the if-block. Moreover, the constant in the test condition must always be on the right-hand side so the test condition reads like regular English, not like Yoda-speak.
For example, change this:
if (false === $branches = $cache->get($cacheId)) {to
$branches = $cache->get($cacheId);
if ($branches === false) {
This pull request has automatically rebased to 4.2-dev.