ENHANCEMENT filtering for large user base sites. #312
Conversation
|
Fixing test fail now. |
camfindlay
force-pushed
the
fix-largesitesupport
branch
from
56f88b4 to
c3b43a7
Compare
|
Unsure why test here is failing, would appreciate some help on this (code is working as expected however good to have tests in place). |
|
Because you are calling a protected method from outside the class. :D |
|
@tractorcow in that case should I simply open the visibility of the method? |
camfindlay
force-pushed
the
fix-largesitesupport
branch
2 times, most recently
from
e401914 to
de65f9a
Compare
|
Updated both the getCandidate___ methods to be public, I can see as these are getMethods they should be accessible on the view layer and hence should both be public methods. |
|
@camfindlay unfortunately that's a semver violation for a non-major release; We can't change method visibility of existing functions without releasing blog 3.0. =/ If you need to test it, have a subclass that exposes those methods just for testing. I.e. have a spy subclass that implements |
|
If I put getCandidateUser back to protected that should be fine then right? getCandidateAuthors is a new method and not breaking any API? Minor version release? |
camfindlay
force-pushed
the
fix-largesitesupport
branch
from
de65f9a to
d6c2593
Compare
|
@tractorcow I've put the method on Blog back to previous visibility. Would this now be a BC PR? |
ENHANCEMENT filtering for large user base sites.
|
Looks good mate. :D |
| @@ -205,7 +213,7 @@ public function getCMSFields() { | |||
| $authorField = ListboxField::create( | |||
| 'Authors', | |||
| _t('BlogPost.Authors', 'Authors'), | |||
| Member::get()->map()->toArray() | |||
| $this->getCandidateAuthors()->map()->toArray() | |||
There was a problem hiding this comment.
Oops, turned out this broke compatibility with php 5.3 since $this is used in a closure. :)
BUG Fix regression in #312 in PHP 5.3
|
To test protected methods you should use ReflectionMethod. It's been done a few times in the core tests if you need an example |
|
True, but should unit tests be covering protected methods, as a matter of practice? I'd rather that they were publicly defined API. If they should be unit tested, and thus have predictable behaviour (that should be predictable) then it should follow the same restrictions that semver place on public API. Otherwise, internal refactoring will mean constantly having to change the assumptions of tests, even when no API has actually changed. I had this trouble with the new But I digress... we are getting into philosophical grounds here. :D |
|
Unit tests are to check units of functionality work as expected. If x goes in y comes out. If the method is changed that now z comes out, well, tests need to be updated (as they would with public methods). Tests aren't there just to check the public API works, they're there to check there aren't regressions at all. |
|
Then it feels like we are treating protected methods as public API, so it may as well be public. |
|
except semver And by that arguement, every function should be public, which isn't the case, and we just kind of accept regressions to protected or private methods as course of life, which seems silly. If someone wants to re-factor the internals of a function to be more efficient we want to know x -> y and not x -> z; that's what tests are for... |
|
I think we simply have a difference in opinion in testing goals. :) I'm going to have a think about this and come back to this discussion later. |
Improvements for #311