[WIP] Fix localization files loading problem #4444
Conversation
* Let's perform tests against PHP 7.1 too * Let's see how PHP nightly behaves * Allow failure of PHP 7.1 * Fix allow_failures TravisCI directive
Newer PHPUnit versions show this warning: Trying to configure method "getCollectionObject" which cannot be configured because it does not exist, has not been specified, is final, or is static Let's fix this warning
* remove media query that was breaking the fieldless inputs * rebuild CSS
* Bootstrap grid elements are breaking fieldless inputs and missing form-group class * tabs to spaces
* Bootstrap grid elements are breaking fieldless inputs * tabs to spaces * translate select options
* Docblocks * Return type string in __toString method
- also add the Selectize remove button
PHP 7.0 won't work without this
…ager-package-updates Feature/entity manager package updates
| { | ||
| parent::__construct(); | ||
|
|
||
| $this->cacheName = $cacheName; | ||
| $this->setApplication(Application::getFacadeApplication()); |
There was a problem hiding this comment.
If we use $app->make or $app->build to create these classes, you can just implement ApplicationAwareInterface and avoid being coupled to the application facade
There was a problem hiding this comment.
Fixed in upcoming commit that I'll add to this PR (#4444)
| $this->cacheLifetime = $cacheLifetime; | ||
| } | ||
|
|
||
| public function setApplication(\Concrete\Core\Application\Application $app) |
There was a problem hiding this comment.
Looks like we already meet the minimum requirements for that anyway.
| if ($this->isLocalizationReady()) { | ||
| return $this->app->make($this->finalCacheName); | ||
| } else { | ||
| return null; |
There was a problem hiding this comment.
I don't think we need to be explicit with null returns
There was a problem hiding this comment.
Maybe because of many years programming in other languages, as for myself I prefer writing return null instead of just return. I know they lead to the same result, but they are conceptually different...
Think for instance at:
- a function like
echothat never returns anything. - a function that parses a string and returns an int if it's valid or null otherwise,
I'd only use return alone in the first case, when functions are not supposed to never return anything.
That's why I suggested this for our .php_cs
| protected function isLocalizationReady() | ||
| { | ||
| if (!$this->localizationReady) { | ||
| if ($this->config->get('app.bootstrap.packages_loaded') === true) { |
There was a problem hiding this comment.
I know why we do this, but just seeing $this->config->get('app.bootstrap.packages_loaded') here makes me sad. We should've made a $status = $app->packageStatus(); or something.
There was a problem hiding this comment.
@KorvinSzanto I'm totally with you. This would solve #4253 (see point 4. of the cause).
| $item = $cache->getItem('zend/'.$normalizedKey); | ||
| if ($item->isMiss()) { | ||
| $cache = $this->getCache(); | ||
| $key = 'zend/'.$normalizedKey; |
There was a problem hiding this comment.
Doesn't look like this variable gets used
There was a problem hiding this comment.
Fixed in upcoming commit that I'll add to this PR (#4444)
| if ($result = $item->set($value, $this->cacheLifetime)) { | ||
| $item->save(); | ||
| $cache = $this->getCache(); | ||
| if ($cache === null) { |
There was a problem hiding this comment.
With this pattern, I'd just do:
if ($cache = $this->getCache()) {
$item = ...
return true;
}
return false;Also seeing the 'zend/'.$normalizedKey) here makes me thing this might benefit from a protected method like $this->getCacheKey($normalizedKey)
There was a problem hiding this comment.
For a cleaner code, I prefer to avoid assignments in if statements.
Indeed, if you read if ($a = 2) { you may think that's wrong, you have to go back to the code and check what the coder wanted.
About getCacheKey: fixed in upcoming commit that I'll add to this PR (#4444)
| @@ -87,12 +138,14 @@ protected function internalGetItem(&$normalizedKey, &$success = null, &$casToken | |||
| */ | |||
| protected function internalSetItem(&$normalizedKey, &$value) | |||
There was a problem hiding this comment.
This is part of zend yeah? I wonder why they pass both arguments by reference :S
There was a problem hiding this comment.
I don't know why, but yes, it's because the base ZF abstract class defines the abstract method this way.
| $item = $cache->getItem('zend/'.$normalizedKey); | ||
| $cache = $this->getCache(); | ||
| if ($cache === null) { | ||
| $result = true; |
There was a problem hiding this comment.
Returning true for this is a little weird
There was a problem hiding this comment.
It depends on the meaning you give.
- If
internalRemoveItemmeans "please be sure that that item does not exist in the cache", we should return true. - If
internalRemoveItemmeans "please remove that item and tell me if you succeeded", we should return false.
I adopted 1, but also 2 makes sense... I'll update my code.
| * | ||
| * @return bool | ||
| */ | ||
| public function flush() | ||
| { | ||
| return Core::make($this->cacheName)->getItem('zend')->clear(); | ||
| return $this->app->make($this->finalCacheName)->getItem('zend')->clear(); |
There was a problem hiding this comment.
this should use the getCache() method so that we're not duplicating code
There was a problem hiding this comment.
getCache returns the cache only if all the translations files are ready. BTW one may want to clear the cache even before that occurs...
|
For a better and more robust handling (solving #4444 too), what about saving the translations state as a property (with getters/setters) of the |
mlocati
changed the title
KorvinSzanto
force-pushed
the
develop
branch
from
938f77d
to
c5e9138
Compare
|
This pull request needs to be rebased and reopened because we did some history rewriting, if you need help with this head into #dev in our slack channel |
This is an alternative solution (for v8 now) to #4443