Add initial barcode validation and tests #2

harikt · 2017-10-01T05:57:30Z

Hi @weierophinney ,

This PR contains Barcode validation .

Some of the tests are commented out which need to be addressed. I am still thinking how we can resolve those.

Are you creating a new feature?
- Why is the new feature needed? What purpose does it serve?
- Add tests for the new feature.
- Add documentation for the new feature.
- Add a CHANGELOG.md entry for the new feature.

harikt

Let me know what all changes you need to merge this PR.

Thank you.

harikt · 2017-10-01T06:21:05Z

src/Barcode.php

@@ -0,0 +1,159 @@
+<?php
+declare(strict_types=1);


I wonder why declare(strict_types=1); is omitted on other classes. Is that purposeful ?

We haven't yet decided if we will require that.

harikt · 2017-10-01T06:36:01Z

test/BarcodeTest.php

+    // {
+    //     $this->expectException(InvalidArgumentException::class);
+    //     $this->expectExceptionMessage('not found');
+    //     $barcode = new Barcode('\Zend\Validate\BarcodeTest\NonExistentClassName');


In the old code this send an InvalidArgumentException before calling getAdapter() . Do we want to do the same ? In that case the adapter is created first even if it may / may not is used.

harikt · 2017-10-01T06:37:23Z

test/BarcodeTest.php

+
+
+
+    // public function testDefaultArrayConstructWithMissingAdapter()


I think we can remove these tests. What do you think ?

Yes, as these ports should already the constructor to accept specific arguments, not an option array.

harikt · 2017-10-01T06:40:25Z

test/BarcodeTest.php

+        $barcode = new Barcode('ean8');
+        $result = $barcode->validate('123');
+        $this->validateResult($result, false);
+        // $message = $barcode->getMessages();


$result->getMessages() returns array of ValidationFailureMessage . If we can keep the key as code of ValidationFailureMessage it may be easy to check this. May be there is another way to resolve this.

The code is part of the validation failure messages.

weierophinney · 2017-10-01T12:56:41Z

@,hatikt - Thanks for starting this! However, per the RFC, this is one of the validators that will get a bridge component, due to the required dependency it had on zend-barcode. If you want to create that component, you can do so under your own user/org, and then open an issue here (or, once we've moved the component to the zendframework org, on that repo) letting us know, so we can bring it into the zendframework org.

The new component would depend on this one and zend-barcode.

harikt · 2017-10-01T17:21:58Z

Hi @weierophinney ,

Thank you for your quick reply. I am a bit confused for I didn't noticed any dependency on zend-barcode when writing a test for the Barcode validation component.

I will look more into the discussion on zendframework/zend-validator#1 and https://discourse.zendframework.com/t/rfc-new-validation-component/208/2 .

weierophinney · 2017-10-02T19:14:28Z

I am a bit confused for I didn't noticed any dependency on zend-barcode when writing a test for the Barcode validation component.

Okay, so it turns out I was wrong! In reviewing the various barcode-related validators, none of them have a dependency on zend-barcode. Additionally, it's not a suggested (or required!) dependency either.

So... keep up the great work, and I'll review when you've finished!

harikt · 2017-10-03T05:06:48Z

@weierophinney I have updated the code. I belive you can do a final review.

weierophinney

Thanks for the changes, @harikt !

I've got a list of new changes for you. Most importantly:

The Barcode class should be immutable. As such, it should accept a concrete adapter via its constructor.
A number of adapters can benefit from some slight refactoring.
The tests need a bit of work, and I've provided some notes on how to improve them.

Thanks for tackling this large set of validators!

weierophinney · 2017-10-03T15:36:06Z

src/Barcode.php

+
+    private $useChecksum;
+
+    public function __construct($adapter = null, bool $useChecksum = null)


Typehint this argument; only accept AdapterInterface instances.

weierophinney · 2017-10-03T15:36:35Z

src/Barcode.php

+     *
+     * @return Barcode\AbstractAdapter
+     */
+    public function getAdapter()


Add a return typehint.

weierophinney · 2017-10-03T15:37:29Z

src/Barcode.php

+                $adapter = 'Ean13';
+            }
+            $this->setAdapter($adapter);
+        }


The above becomes unnecessary if a concrete instance is provided via the constructor.

weierophinney · 2017-10-03T15:38:58Z

src/Barcode.php

+     * @return Barcode
+     * @throws Exception\InvalidArgumentException
+     */
+    public function setAdapter($adapter)


Remove this method; validators should be immutable, with the exception of message templates.

weierophinney · 2017-10-03T15:39:30Z

src/Barcode.php

+
+            if (! is_null($this->useChecksum)) {
+                $adapter->useChecksum($this->useChecksum);
+            }


Move these lines to the constructor.

Hi @weierophinney ,

If we are passing the concrete adapter, do we really need to pass the $useChecksum . There is no place where this variable need to be used. Can't we remove this ?

If the main barcode validator doesn't and/or shouldn't override what's in the adapter, then we can remove it.

weierophinney · 2017-10-03T16:12:05Z

test/BarcodeTest.php

+
+    public function testInvalidChecksumAdapter()
+    {
+        require_once __DIR__ . "/_files/MyBarcode1.php";


These require_once calls can go away once you update the various test adapters to use the ZendTest namespace. Doing so will require adding a new autoload-dev rule to composer.json so they may be autoloaded.

weierophinney · 2017-10-03T16:12:24Z

test/BarcodeTest.php

+     * @dataProvider validationProvider
+     */
+    public function testValidateReturnsExpectedResults(
+        $adapter,


Typehint this argument.

weierophinney · 2017-10-03T16:13:05Z

test/BarcodeTest.php

+    public function testInvalidChecksumAdapter()
+    {
+        require_once __DIR__ . "/_files/MyBarcode1.php";
+        $barcode = new Barcode('MyBarcode1');


Here and throughout this file, you'll need to switch from using string "short" names to new <CLASSNAME>.

weierophinney · 2017-10-03T16:13:49Z

test/BarcodeTest.php

+        $this->expectExceptionMessage('does not implement');
+        require_once __DIR__ . "/_files/MyBarcode5.php";
+        $barcode->setAdapter('MyBarcode5');
+    }


This method can be removed after the constructor changes are made, and the setAdapter() method is removed.

weierophinney · 2017-10-03T16:14:32Z

test/BarcodeTest.php

+        $this->validateResult($barcode->validate('123'), true);
+        $this->validateResult($barcode->validate('123a'), false);
+
+        $barcode->useChecksum(true);


Once the change to useChecksum() is made in the Barcode class, you'll instead change the flag on the adapter within these tests.

harikt · 2017-10-03T16:42:44Z

Thank you for the detailed review @weierophinney . I will send an update in the coming days.

harikt · 2017-10-05T14:52:48Z

Hi @weierophinney ,

Have updated code as requested and added documentation ( copied from old barcode validator , updated with the current ones )

Thank you.

I will add the documentation

harikt · 2017-10-06T14:47:28Z

src/Bitwise.php

+
+use Traversable;
+
+class Bitwise extends AbstractValidator


Oh shit, I committed this file accidentally 👎 . @weierophinney what do you want to do about this ?

Remove this file from this PR and send a different PR .

or add unit test / fix for this Bitwise one ?

Remove it.

I've taken the liberty of rebasing your branch off of current master, and I did an interactive rebase to remove the Bitwise class file from the commit where it was introduced.

So do I want to do anything here ? I am a bit confused with the sentence.

I've taken the liberty of rebasing your branch off of current master, and I did an interactive rebase to remove the Bitwise class file from the commit where it was introduced.

So did you removed the Bitwise class or do you want me to remove it ? or when you are doing the rebasing you will do ?

- Use 2018 for copyright date - Use `Result` named constructors - Use `TestAsset` namespace for barcode test classes

harikt · 2018-06-12T04:55:42Z

It looks like you have removed the Bitwise class. Is there anything else that makes it a blocker for not merging ? I got only one email, but this one still shows requested changes. Not sure what are the changes requested newly. I assume all changes have been already integrated earlier itself. Let me know if I have missed anything.

Extracts checksum functionality to a new interface, `ChecksummableInterface`, which defines the various methods for determining if a checksum is in use, as well as how to invoke it. Most checksummable barcode validators now accept a boolean flag, `$useChecksum` to its constructor, defaulting it to whatever the default behavior should be (generally `true`, indicating checksum validation should occur). Some override `useChecksum()` to determine if the value requires a checksum validation, usually based on length or an initial or final character in the barcode. **ALL** barcode validators now have only constructor injection for defining behavior. This ensures they are always in a consistent state. Finally, all barcode validators now have strict_types enabled, which caught a number of errors in how the various checksum operations were being calculated and compared.

Extracts a trait from each of Code39 and Code93 for the purposes of validating the checksum, and then updates the `ext` variants to allow checksum verification.

- Puts usage docs at top - Ensures each class documents any constructor arguments present - Updates examples to match code usage

Using `ZendTest\DataValidator\Barcode\TestAsset` namespace for custom barcode validators, which does not require an additional rule.

- Removes unnecessary result assignments. - Combines conditionals - Extracts the method `getAllowedLength()`, and simplifies the logic (as it was doing the same work as `implode()`)

Removes redundant "barcode" verbiage.

weierophinney · 2018-06-19T21:45:14Z

@harikt Sorry for the delay getting back to you!

I'd started on some refactoring, but then had other obligations last week that prevented me from finishing, as well as providing you details of what I was doing.

What I've done, in general, is as follows:

Removed all public setters, in favor of constructor arguments. This ensures that the various barcode adapters, and the Barcode validator itself, remain stateless (one of the primary goals of the entire component!).
I've separated out the "checksum" functionality into an additional interface and trait. Adapters that provide checksum capabilities now implement that interface (optionally via the trait). The Barcode validator checks to see if the adapter implements that interface, and if a checksum validation is required, before attempting to validate the checksum. Checksum methods were moved either into the specific adapters, or, if used in more than one location, into traits. This change greatly simplified the AbstractAdapter.
Added declare(strict_types=1) throughout all new classes. This helped me identify and correct some problems with length, character, and checksum validations that have been lingering since they were originally written.

I've also updated the documentation to list out any constructor arguments, and to push the usage example to the top of the page.

Thanks a ton for your work on this!

Close #2

harikt · 2018-06-20T04:35:35Z

Sorry for the delay getting back to you!

No issues.

The problem with too much delay is we all will be carried away to other stuffs and forget about what we did and why we did ( Probably a failure at my end to document things ) . I always forget in 2-3 days and find it hard to get back to it. This is not a complaint. Just telling about my personal thing.

And thank you for your good work here. Any plans about release date etc ?

harikt commented Oct 1, 2017

View reviewed changes

weierophinney requested changes Oct 3, 2017

View reviewed changes

harikt commented Oct 6, 2017

View reviewed changes

harikt and others added 6 commits June 11, 2018 16:33

Add initial barcode validation and tests

db27018

update test

ba3532c

update tests and incorporate feedbacks

faa90f2

Incorporate feedbacks from @weierophinney

318b772

Add documentation on barcode validator

421a373

Update to follow current changes

d5137d0

- Use 2018 for copyright date - Use `Result` named constructors - Use `TestAsset` namespace for barcode test classes

weierophinney force-pushed the barcode branch from 463c3e3 to d5137d0 Compare June 11, 2018 21:47

weierophinney added 7 commits June 19, 2018 14:58

Adds checksum support to Code39ext and Code93ext barcode validators

95b5e66

Extracts a trait from each of Code39 and Code93 for the purposes of validating the checksum, and then updates the `ext` variants to allow checksum verification.

Updates documentation for barcode validators

a6a0a7b

- Puts usage docs at top - Ensures each class documents any constructor arguments present - Updates examples to match code usage

No need for additional autoload-dev rule

29d5c3a

Using `ZendTest\DataValidator\Barcode\TestAsset` namespace for custom barcode validators, which does not require an additional rule.

Refactor Barcode::validate to simplify it

903aaf8

- Removes unnecessary result assignments. - Combines conditionals - Extracts the method `getAllowedLength()`, and simplifies the logic (as it was doing the same work as `implode()`)

Fixes CS issue reported by phpcs

1311a02

Provides new values for Barcode validator constants

10290e0

Removes redundant "barcode" verbiage.

weierophinney approved these changes Jun 19, 2018

View reviewed changes

weierophinney merged commit 10290e0 into weierophinney:master Jun 19, 2018

weierophinney added a commit that referenced this pull request Jun 19, 2018

Merge branch 'feature/2-barcode'

e62fd6b

Close #2

harikt deleted the barcode branch June 20, 2018 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial barcode validation and tests #2

Add initial barcode validation and tests #2

harikt commented Oct 1, 2017 •

edited

Loading

harikt left a comment

harikt Oct 1, 2017

weierophinney Oct 1, 2017

harikt Oct 1, 2017

harikt Oct 1, 2017

weierophinney Oct 1, 2017

harikt Oct 1, 2017

weierophinney Oct 1, 2017

weierophinney commented Oct 1, 2017

harikt commented Oct 1, 2017

weierophinney commented Oct 2, 2017

harikt commented Oct 3, 2017

weierophinney left a comment

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

harikt Oct 5, 2017

weierophinney Oct 5, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

weierophinney Oct 3, 2017

harikt commented Oct 3, 2017

harikt commented Oct 5, 2017

harikt Oct 6, 2017

weierophinney Jun 11, 2018

harikt Jun 12, 2018

harikt commented Jun 12, 2018

weierophinney commented Jun 19, 2018

harikt commented Jun 20, 2018




		// public function testDefaultArrayConstructWithMissingAdapter()


		private $useChecksum;

		public function __construct($adapter = null, bool $useChecksum = null)

		@@ -0,0 +1,159 @@
		<?php
		declare(strict_types=1);

Add initial barcode validation and tests #2

Add initial barcode validation and tests #2

Conversation

harikt commented Oct 1, 2017 • edited Loading

harikt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Oct 1, 2017

harikt commented Oct 1, 2017

weierophinney commented Oct 2, 2017

harikt commented Oct 3, 2017

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harikt commented Oct 3, 2017

harikt commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harikt commented Jun 12, 2018

weierophinney commented Jun 19, 2018

harikt commented Jun 20, 2018

harikt commented Oct 1, 2017 •

edited

Loading