-
Notifications
You must be signed in to change notification settings - Fork 161
Conversation
foreach ($this->serviceLocator->get('BjyAuthorize\RoleProviders') as $provider) { | ||
$this->addRoleProvider($provider); | ||
} | ||
$cache = StorageFactory::factory(array( |
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.
Don't create the cache here - instead, inject it and allow a null cache if none is set.
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.
Inject it from where?
Here? https://github.com/bjyoungblood/BjyAuthorize/blob/master/src/BjyAuthorize/Service/Authorize.php#L83
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.
Ideally, as a cosntructor param
Moved $cache initialization to __constructor
Moved $cache initialization to __constructor
@@ -35,12 +35,14 @@ | |||
"require-dev": { | |||
"doctrine/common": ">=2.3,<2.5-dev", | |||
"zendframework/zend-developer-tools": "0.*", | |||
"zf-commons/zfc-user": "0.*" | |||
"zf-commons/zfc-user": "0.*", | |||
"zendframework/zend-cache": "~2.1" |
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.
Should work also for ~2.0
Various fixes
Fixed $config definitions in AuthorizeFactory
Tests? |
I have not enough time to learn how to test this :( |
News on this one? |
I don't know how to test! :( Oscar Fanelli Email: [email protected] Il giorno 30/mag/2013, alle ore 18:20, Marco Pivetta [email protected] ha scritto:
|
Hello, i really want a cache system. I have a lot of database queries because of BjyAuthorize getting the roles from the database. You don't know how to test because you didn't ever use PHPUnit? |
Go for it! I'm busy for at least another week...
|
@cbergau the cache system is working and I "manually" test it with some of my website (with a lot of old "var_dump"), but however is missing a real test with phpunit, because I have no time to study it, even though they are really very few lines of code. |
I just looked at your commits. I think its good. But i think its impossible to pass any options to the cache adapter right? Have a look at the Zend\Cache\StorageFactory. I think you should use the StorageFactory to create the cache instead of using "new". BTW: You should definitely spend some time into PHPUnit, you won't regret :-) |
use Zend\ServiceManager\ServiceManager; | ||
|
||
/** | ||
* Test for {@see \BjyAuthorize\Service\AuthorizeAwareServiceInitializer} |
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.
This @see
is wrong
Okay, i can not explain why the Travis build fails, because i did not change any of the failing code :/ using PHPUnit 3.7.21 and PHP 5.4 |
@cbergau that's not up to you - I think zendframework/zend-http is missing. I'll try locally tomorrow |
Any news? :-) |
Nope, sorry... Wanted to check it in the midday break, but failed to
|
@@ -8,8 +8,10 @@ | |||
|
|||
namespace BjyAuthorize\Service; | |||
|
|||
use Zend\Cache\StorageFactory; |
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.
You can remove the StorageFactory and the ConsoleRequest in the use block.
{ | ||
$this->acl = new Acl(); | ||
|
||
foreach ($this->serviceLocator->get('BjyAuthorize\RoleProviders') as $provider) { |
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.
Why not remove those loops and allow addRoleProvider();
to accept arrays too? Same for the others.
It's a kind of small BC change, but the end user behaviour remains the same
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.
Not part of the cache stuff.
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.
Ah, saw it, you've simply moved existing code ...
Hey, if you want, i can do try to find out whats wrong with the build, if this speeds it up :-) |
Manually merged (and squashed) |
This is a nice example of open source collaboration.. great work everybody.! |
Add a cache system based on APC during the Authorize service load.