-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[WIP] Zend\Filter harmonization (Issue 5119) #5436
Conversation
I think most of those changes are BC. I've done already a lot of work to clean filters for ZF3 (#5097), maybe you could directly do PR to my branch so that those changes are also included in the refactor branch? What do you think? |
@bakura10 most of the changes are definetly no BC breaks. The only difference is, that there is now always a check of the filtered values, before filtering and no error is triggered. For me both ways are okay. ZF2 or ZF3....someone else shall decide 😄 |
Speaking of the behavior of filters, keep this in mind. |
That's an interesting thing @texdc . In fact, hydrator filters are... well... filters. They don't hydrate/extract unwanting filter. In fact, what you would want would be to rename Filter component to Sanitizer component? I'm not against this idea, however I'm wondering if it would be a good thing to do. People are accustomed to using the Zend\Filter component since... many many many years. Changing this now would indeed confuse a lot of people. Again, the tradeoff between correctness, usage and simplicity is not an easy task. |
@@ -24,6 +24,10 @@ class Decrypt extends Encrypt | |||
*/ | |||
public function filter($value) | |||
{ | |||
if(!is_string($value)){ |
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.
add space after if before open parenthesis and after close parenthesis before open bracket.
@samsonasik coding standards should be fine now. (i hope i didn't missed a spot...is there also a automatically fixer?) About the notes for the "null" check. It's the same behaviour....null is not a scalar, so it return itself (which is null) /* original */
if (null === $value) {
return null;
}
if (!is_scalar($value)) {
return $value;
}
/* new */
//the same, if the value is null -> it will return null...
if (!is_scalar($value)) {
return $value;
} _Missing_ |
@weierophinney all should be fine now.... about null check again: it's also tested here: |
In my second last commit, i fixed the call in invoke. I don't understand why null have to be passed, but i let now pass null again in my last commit. namespace ZendTest\Filter;
use Zend\Filter\Decompress as DecompressFilter;
/**
* @group Zend_Filter
*/
class DecompressTest extends \PHPUnit_Framework_TestCase
public function testCompressToFile()
{
$filter = new DecompressFilter('bz2');
$archive = __DIR__ . '/../_files/compressed.bz2';
$filter->setArchive($archive);
$content = $filter->compress('compress me');
$this->assertTrue($content);
$filter2 = new DecompressFilter('bz2');
$content2 = $filter2($archive);
$this->assertEquals('compress me', $content2);
$filter3 = new DecompressFilter('bz2');
$filter3->setArchive($archive);
$content3 = $filter3(null);
$this->assertEquals('compress me', $content3);
} |
@weierophinney everything fine or not? |
@ThaDafinser Yes, I think so -- need to review again. (Been busy getting Apigility ready for beta...) |
This would be good to have since currently adding filters in apigility causes breakage if the inputs come in as something the filters cannot deal with. |
[WIP] Zend\Filter harmonization (Issue 5119) Conflicts: library/Zend/I18n/Filter/NumberFormat.php
- Detailing what the PR accomplishes, as it's a slight change in behavior.
Merged to develop for release with 2.3.0. |
Yiiiha :-) finally |
@ThaDafinser Thanks for doing this, it solves a big problem I was having |
…ZendFilter [WIP] Zend\Filter harmonization (Issue 5119) Conflicts: library/Zend/I18n/Filter/NumberFormat.php
…ZendFilter [WIP] Zend\Filter harmonization (Issue 5119) Conflicts: library/Zend/I18n/Filter/NumberFormat.php
Filters have currently different behaviours, like:
See issue here: #5119 (comment)
this PR should harmoize to one behaviour.
Todo list
not possible to check?
as a user defined function is awaited (could be everything)not possible to check?