Closed webignition closed 6 years ago
The contribution docs state that changes must add tests to be accepted.
Just acknowledging here that this change does not add any tests. I believe no new tests are needed. That the current tests pass feels sufficient to me since no new functionality is being added.
Why do you need the refreshRules
in the interface ? IMHO only the getRules
method is really needed. Forcing a builder to refresh means your are forcing a builder to do some caching too ?
Short version of why I specifically need this: this needs to be mocked in unit tests for code that makes use of a Manager
object for the refreshing of locally-cached PSL data. Such unit tests shouldn't need to make filesystem calls or HTTP requests.
The current Manager
implementation already has a refreshRules()
method and does do some caching and so I don't see the addition of ManagerInterface::refreshRules()
to present any problems.
That said, it could be argued that the functionality provided by Manager::getRules()
and Manager::refreshRules()
don't need to be part of the same interface. That the current Manager
implementation provides both of these methods does not necessarily mean that both methods belong under the same interface.
When considering ManagerInterface
as being an interface for the public methods currently provided by Manager
, having both getRules()
and refreshRules()
as methods of the interface makes sense.
If you don't think that refreshRules()
belongs in the same interface as getRules()
(and I'm now wondering the same myself), perhaps getRules()
and refreshRules()
belong more correctly as methods of separate interfaces where Manager
happens to implement both interfaces?
We could possibly factor out ManagerInterface::getRules()
to RuleProviderInterface::getRules()
and ManagerInterface::refreshRules()
to RuleMaintainerInterface::refreshRules()
.
I'm not sure if doing so is getting to abstract and is not over-engineering at present.
If you don't think that refreshRules() belongs in the same interface as getRules() (and I'm now wondering the same myself), perhaps getRules() and refreshRules() belong more correctly as methods of separate interfaces where Manager happens to implement both interfaces ?
TBH that's what I have always thought. Because the only thing the getRules
method guarantee is that it does returns a Rules
object from an URL. The added value of the "Manager" implementation is that it is "smart" enough not to make a HTTP Request for each demand, otherwise Rules::createFromPath
would be enough.
How the Manager does it, it's via the refreshRules
but nothing prevents another manager from doing it differently.
I was previously skimming through the readme to find out how best to use the latest version of the package, having previously been on 1.x.
I can handle the retrieval and local-caching of PSL data directly without the need for using a Manager
instance, making the need for this change somewhat less relevant.
@webignition indeed ... that is also true. the Manager
is an example on how to do it properly but I still think that depending on the requirements you may use other means to create/build a Rules
object. I tried to make the code less dependent of the Manager
in contrast to previous versions where the Manager was always required.
Introduction
Fixes #225
Proposal
Describe the new/upated/fixed feature
Creates
ManagerInterface
whichPdp\Manager
then implements. This allows mocks to be easily created againstManagerInterface
for use in unit tests in external projects that make use ofPdp\Manager
instances.See PR #225 for detail.
Backward Incompatible Changes
No backward incompatible changes.
Targeted release version
The next release version, whatever that might be.
Since this is an incremental change adding no new functionality, a minor release 5.3.1 seems fine to me.
PR Impact
No changes to the public API
Open issues
None