Refactor so that persistence is extendable. #116
Conversation
UrlManager.php Outdated
| } | ||
| | ||
| /** | ||
| * Persists language in session and cookie. |
There was a problem hiding this comment.
Should be
@param string $language the language code to persist UrlManager.php Outdated
| } | ||
| | ||
| /** | ||
| * Attempts to load language from persistent storage. Returns null if not found. |
There was a problem hiding this comment.
Could be combined into a single line:
@return string|null the persisted language code or null if none found 
| @ddinchev Yes, it seems to make sense. The library has grown over the years and the code has become bloated. But let's refactor one part at a time. Sidenote: If you happen to find a quick solution for failing travis tests that would be great. I remember there was some issue about missing inputmask dependency at yiisoft, but I don't have time to look into it. I want to make sure, that the tests pass before I merge this. |
| @ddinchev Ah, thanks, much better :). If you could check my two little code comments I'd merge. |
| @mikehaertl thanks for the blazing-fast responses! I've amended my changes. I've committed the |
Hmm, sorry, I think I disagree on this one. It may make sense for complete apps. But as this here is rather a library I don't want to force too specific version constraints on the user. The dependency configuration in If you really think this needs further discussion please open an issue for it. |
| TravisCI failed because GitHub blocks the tons of requests to resolve the dependencies, thus requiring an auth token to proceed. With a lock file, it installs the correct versions directly. Could you read the documentation link I've added? They have articulated the arguments to do so better than I could. When the library is installed as a dependency, it does not install any of the dependencies specified in the composer file - with or without Further, this is what Composer team says about
|
| Hmm, isn't the problem rather something with fxp-assetplung (Did i mention how much I hate this thing?) and jquery inputmask? I think you even proposed some workaround in this issue here: yiisoft/yii2#14275 (comment) We should try something similar maybe. But I'm still against having Put another way |
| To speed things up a bit: I'll have a look at the broken travis tests myself. If you remove the corresponding commits here I'll merge the refactoring part. Please also remove the composer.lock again. As said before - if really we can discuss this in a dedicated issue or PR. |
| @ddinchev I've fixed the travis tests now by adding the asset-packagist repo. This is the fix they also used with official yii2 plugins: yiisoft/yii2-bootstrap@e6a20fa#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780 |
| Thanks @mikehaertl! I've removed my changes related to I did point to the documentation that specifically said it's still a good idea to commit for libraries that are intended to be included in projects (like this one is) and does not have any side effects. Yii2 framework itself has the |
Yes, but official yii2 libraries don't:
The main reason is, that you would have to update the |
| Just released 1.4.13 containing this change. Thanks for your help. |
2 participants
I've moved out the logic to persist and load persisted language in protected methods. This way they can be overridden - I needed that to integrate the component with an existing application i18n component that has its own storage.
PS: I'd be happy to break down all of the lengthy methods into smaller ones so they are more readable and extendable if you would accept such a change (no logic altered, just refactorings)?