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
Validate premium feature license correctly when cancelled #13064
Conversation
core/Plugin/Manager.php
Outdated
$info = $newPlugin->getInformation(); | ||
$hasInternet = Config::getInstance()->General['enable_internet_features']; | ||
|
||
if (!empty($info['price']['base']) && $hasInternet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We check this only for premium features, when the feature is activated, and only when the instance has the marketplace (we wouldn't be able to get the Marketplace class otherwise etc) and internet features enabled.
Found an issue while trying to test: if you have a plugin available that isn't installed or activated, and requires special configuration before it can be activated (like having a central database config defined), then the check will result in trying to install this plugin & failing. Seems to be because |
Cheers, I'll try to have a look into this some day. Might get quite tricky to workaround it. |
Was thinking a possible solution would be to use a scheduled task to check plugin status & then in Plugin\Manager just check the result. |
Was good you found the issue. I now implemented a new method that only checks the license so we also avoid checking lots of other not needed things like whether a plugin is activated, bogus, requirements, ... The problem was that it tried to execute a plugin update check under circumstances which then executes the loading of all plugins. |
My suggestion was just that, a suggestion, I wouldn't take it personally if there was a better one ;) |
fyi l also disabled the check for developers as it can get slow without having a cache. |
fyi: the reason I didn't use the task in the first place is that it can get tricky with caching it in tracker mode. eg the tracker would need to set a DB option but this means we would need to cache it in general tracker cache. This in turn means it gets all a bit complicated and am not sure if we would initialize that cache too early before all plugins are loaded etc. In tracker mode with dev enabled or so I'm worried it could even end in an endless loop... A workaround be to set an option in the task and then cache it within plugin\manager through lazy cache as it is right now but then we could as well keep it simple and maybe not do it through a task? I will have a think again... |
Will tag as Pull Request WIP (feel free to change it back now or whenever it should be reviewed again) |
@diosmosis I've tried to implement it in a task but it's not really working. The problem is that in the task I would need to get a list of all activated (or loaded) plugins. However, I cannot really get the loaded/activated plugin because it would be not loaded in I would basically need to add a flag to the plugin manager to "ignore" the license check and everything starts getting even more complicated. I've made a few more minor changes though that make the code a bit more readable and faster. |
// should currently load between 10 and 27 plugins | ||
$this->assertLessThan(27, count($this->manager->getLoadedPlugins())); | ||
// should currently load between 10 and 28 plugins | ||
$this->assertLessThan(28, count($this->manager->getLoadedPlugins())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me a while to figure out why this increased but that's because Marketplace is now a tracker plugin
@diosmosis increased the cache TTL and fixed tests. Setting it to "needs review" again. |
…#13064) * Check license correctly and disable if needed * make sure instance has internet features * make sure to verify it in tracker mode * because the text changed we need to change the translation key * prevent update check from being executed * simplify and improve performance * set caching time to 6 hours and invalidate cache when viewing subscriptions * fix tests * update ui screenshot
So far we did not consider a cancelled subscription as "missing license", only when no license existed at all. Also we now deactivated a plugin when the license is missing. We check the license once per hour. I would cache it for longer but the problem is, if someone renews the license then it would take eg 12 hours to activate the plugin. Also I wanted to initially use the eager cache which would be slightly faster, however the eager cache is being cached for 12 hours...
Tried to write a test for this but it's quite hard.