Skip to content
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

Activator #790

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ryssbowh
Copy link

commented Jul 7, 2019

Hi,

as discussed here : #789

@mikemand
Copy link
Contributor

left a comment

A few nitpicks. Other than that, it looks good to me.

Did you account for the change from minutes to seconds that Laravel v5.8 did to the cache system?

@@ -166,7 +168,7 @@
'cache' => [
'enabled' => false,
'key' => 'laravel-modules',
'lifetime' => 60,
'lifetime' => 60

This comment has been minimized.

Copy link
@mikemand

mikemand Jul 7, 2019

Contributor

Why? Trailing commas is pretty much a PSR requirement, which this package follows.

'class' => FileActivator::class,
'file' => storage_path('installed_modules'),
'cache-key' => 'activator.installed',
'cache-lifetime' => 604800

This comment has been minimized.

Copy link
@mikemand

mikemand Jul 7, 2019

Contributor

Needs trailing commas.

@ryssbowh

This comment has been minimized.

Copy link
Author

commented Jul 7, 2019

Yes, the cache lifetime for the file activator is in seconds.

Trailing commas... don't know when my brain will get used to that :)

@nWidart
Copy link
Owner

left a comment

Thank you for the contribution, looks good!
I have a couple of comments related to consistency and clarification. 😄

Also, the tests are failing.

}
/**
* Get installed cache

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

This comment looks a bit unclear to me. From the comment alone, I expected this to return laravel's cache implementation.

*
* @var array
*/
protected $installed;

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Could this be renamed to $installedModules. I was hesitating if this was a boolean or not.

Also what's an "installed module"? Is it an active module? If so, it might be more clear to use activeModules instead.

This comment has been minimized.

Copy link
@ryssbowh

ryssbowh Jul 8, 2019

Author

The way I see it, an installed module is a module that exists on the disk, it then has an activation status (1 or 0), both are stored by this activator.

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Hm, thanks that explains the keyword a bit. So this contains an array of all modules? If so I'd still rename it installedModules to be explicit.

class FileActivator implements ActivatorInterface
{
/**

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Why are fields/methods protected instead of private? Could you make all protected fields and methods private?

*
* @return array
*/
protected function getCached()

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Maybe the method could be named getCachedModules?

It seems like this doesn't always returns cached modules. If the cache is disabled, it won't from the looks of it, which doesn't seem to be the responsibility of this method, based on its name.

Actually, the only usage of this method is the constructor, so maybe this shouldn't mention cached at all and just be called getInstalledModules or getActiveModules.

}
/**
* Forgets the installed cache

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Comment seems weird to me, we don't delete cache, we delete the contents of the cache for a specific key in this implementation.
I would call this flushCache or clearCache.

*
* @param Module $module
*/
public function delete(Module $module);

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

There's delete and disable? What's the difference?

This comment has been minimized.

Copy link
@ryssbowh

ryssbowh Jul 8, 2019

Author

This was only to make up for the delete method of the Module class. I assumed it's just a helper for developers or tests since I don't see any reference of it (?).
It basically deletes any activation status for that module (active or not)

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

I see ok, that makes sense. I believe the delete method on the module class is there by request from the community, there could a future delete command that uses that too actually.

*
* @return $this
*/
public function setActive($active)

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Let's typehint the argument to boolean?

This comment has been minimized.

Copy link
@ryssbowh

ryssbowh Jul 8, 2019

Author

I'd agree. But this argument comes from the Module class and it's not type hinted there, should I type hint it there too ?

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Hm, this can still be type hinted without changing the Module class no? I think that would be a first step.
This package existed before scalar type hints in php so I'm only now moving towards them over time.

This comment has been minimized.

Copy link
@ryssbowh

ryssbowh Jul 8, 2019

Author

Ok, to be consistent with the Module class, I'll have to type hint them as integers even though they can only be 0 or 1. And they will have to be changed later, is that what you mean ?

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Indeed, now that I think of it, maybe there's no need to have them as int, as there's no other status other than 0 or 1. Meaning the could be booleans.
If possible you can typehint on both sides, if not, don't worry I'll do that later on myself.

*/
public function setActive($active)
{
$this->active = $active;

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

Since this is a boolean, please rename to isActive.

*
* @return $this
*/
public function setActivator($activator)

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

You can typehint the interface here too.

$this->app->alias(Contracts\RepositoryInterface::class, 'modules');
$this->app->alias(Contracts\ActivatorInterface::class, 'modules.activator');

This comment has been minimized.

Copy link
@nWidart

nWidart Jul 8, 2019

Owner

I wouldn't create the alias here, I'm trying to move away from this practice.

@nWidart

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2019

This is a breaking change so it'll have to be for the next major release.

@nWidart

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2019

It looks like there's no tests at all for this new FileActivate class, this should be added.

@ryssbowh

This comment has been minimized.

Copy link
Author

commented Jul 9, 2019

Quite struggling with the tests as many tests (FileRepository, Module...) change module's activation statuses now, making the next tests assertions fail as the statuses are written in a separate file that is common to all tests, example :

LaravelFileRepositoryTest->it_returns_all_enabled_modules() should expect 0 as no modules have been activated for that test, but a previous test has activated one, so it fails.

what would be the way to go you think :

  • Each test that by its actions create a statuses file should delete it when it ends ?
  • Or the following tests should take into account the previous tests activation statuses ?
  • something else ?
@nWidart

This comment has been minimized.

Copy link
Owner

commented Jul 9, 2019

Each test should be independent of one another so I think deleting the file after each test should be good.

@ryssbowh

This comment has been minimized.

Copy link
Author

commented Jul 9, 2019

A few things about the last 2 commits :

  • removed tests/stubs/valid/DisabledModule as status is not stored within the module folder anymore

  • removed all statuses within tests/*/module.json

  • I've not tested all the Lumen parts of these commits

  • added a reset method to ActivatorInterface as I needed to clear data after the tests

  • changed all statuses typehints to bools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.