Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Fix - missing messageTemplates in ExcludeMimeType validator #92

Merged
merged 3 commits into from
Jun 23, 2016
Merged

Fix - missing messageTemplates in ExcludeMimeType validator #92

merged 3 commits into from
Jun 23, 2016

Conversation

michalbundyra
Copy link
Member

Fix for bug reported in #90

@@ -51,6 +51,10 @@ public function testBasic($options, $isValidParam, $expected)
$validator = new ExcludeMimeType($options);
$validator->enableHeaderCheck();
$this->assertEquals($expected, $validator->isValid($isValidParam));
if (!$expected) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please instead add the expected getMessages return value on the data provider.

*/
public function testBasic($options, $isValidParam, $expected)
public function testBasic($options, array $isValidParam, $expected, array $messages = [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to have all possible values in the data provider for get the whole picture. Please remove the default value.

* @var array Error message templates
*/
protected $messageTemplates = [
self::FALSE_TYPE => "File has an incorrect mimetype of '%type%'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think INCORRECT_TYPE it's a better name. Why FALSE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant was defined, the same constants name are in parent class MimeType, I've added only messages.
We can change it, maybe it is even better, but for consistency I'd prefer to keep FALSE_TYPE ...
And it's not in the scope of the issue ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. You're right.

@Maks3w Maks3w self-assigned this Jun 23, 2016
Maks3w added a commit that referenced this pull request Jun 23, 2016
Maks3w added a commit that referenced this pull request Jun 23, 2016
@Maks3w Maks3w merged commit 151955a into zendframework:master Jun 23, 2016
Maks3w added a commit that referenced this pull request Jun 23, 2016
@michalbundyra michalbundyra deleted the fix/exclude-mime-type-validator branch June 23, 2016 10:43
@weierophinney weierophinney added this to the 2.8.1 milestone Jun 23, 2016
@weierophinney
Copy link
Member

@Maks3w Please don't forget to update the CHANGELOG when merging... (I'll do it now, so I can release this bugfix.)

weierophinney added a commit that referenced this pull request Jun 23, 2016
- Added entry for #92
- Added date for 2.8.1
@Maks3w
Copy link
Member

Maks3w commented Jun 23, 2016

I was waiting for add more patches to the release

@weierophinney
Copy link
Member

No need to wait on releases anymore! :-) My rule of thumb is that if I
won't complete reviewing patches in the same session, release immediately,
and do the other patches in the next release. This is also why it's
important to do change logs on merge; it ensures nobody has to look up
what's been merged when doing a release.
On Jun 23, 2016 10:47 AM, "Maks3w" [email protected] wrote:

I was waiting for add more patches to the release


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#92 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABlV4nl-pdgg_JwiDgcSHxlaU3-Mz1Nks5qOqqRgaJpZM4I59O0
.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants