picqer / php-barcode-generator

Barcode generator in PHP that is easy to use, non-bloated and framework independent.
GNU Lesser General Public License v3.0
1.67k stars 402 forks source link

Feature/2.0 #76

Closed griffithben closed 5 years ago

griffithben commented 5 years ago

Possible 2.0 Release

New Usage

$generator = new BarcodeGenerator('012345678', BarcodeType::TYPE_CODE_128, BarcodeRender::RENDER_JPG);

// Generates our default barcode with width=2, height=30, color=#000000
$generated = $generator->generate();

// Generates the same code with style updates
$generated = $generator->generate(4, 50, '#FFCC33');

Things still needed

griffithben commented 5 years ago

@casperbakker Thoughts?

griffithben commented 5 years ago

@casperbakker Anything I can do to help with this?

casperbakker commented 5 years ago

Hi @griffithben,

Thanks for the work you put into this. But I find it very hard to decide what to do with this. It is basically unmergable for me, because everything is changed and without an explanation as to why you made the change.

It think this package can totally be made better in a lot of ways. I am also in the process of at least splitting up the generator in separate files and untangle everything. The base of this project is from a PDF generator and we extracted the barcode generating part of it. The messy code is just as it was, but it is so tangled into each other that is the first hard step.

But with this big PR without much explanation to why and without discussing this move before you made all this effort makes it hard for me to judge this PR.

Without looking in-depth to the code, the thing that I dislike is using 1 BarcodeGenerator class and feeding it constants. I think the original way with different classes based on the outputs is better, I think it also needs different classes for the different barcode types. So you have 2 types of classes, and you feed one class into the other and together they render the barcode. I think that is a more flexible and less tied approach.

Maybe you can explain your choices to me a bit more, to help me with the decision.

Best, Casper

griffithben commented 5 years ago

Thanks for getting back to me @casperbakker!

Yes, I agree this is a huge PR and might have served better as a separate repo/branch to discuss the changes instead of a PR into master.

The main goal of this was to break up the huge BarcodeGenerator.php file. While looking through it, it was kind of difficult to follow along. None of the original code from this file has been changed, but the methods have been moved into appropriate class names in src/Barcode/Types. This is also why I haven't touched some of the changed requests @peter279k mentioned. I also wanted to break up the file types into what I referred to as Renders.

From here, I thought it would be useful to follow a factory pattern of generating the render and types of barcodes I would need on instantiation. Doing this instead of needing to refer to different classes like BarcodeGeneratorSVG, BarcodeGeneratorPNG, etc. This way the consumer would only have to know of a single class BarcodeGenerator

In doing this, I also unified the color parameter of the generate method. Between the different render types, some were following an RBG array or a string hex value. I unified this on hex and convert to RBG when needed depending on the render type.

I do see your point with the constants. In hindsight, I could see doing a dependency injection of the Render and Type objects on the instantiation of the BarcodeGenerator class.

I do appreciate the work you put into this project and hope my huge PR wasn't too much of a pain. Like I said, the main goal of this was to just break it down into smaller manageable chunks.

Perhaps this would be better suited as a new branch on the repo? Then build upon that until a releasable version 2 can be made?

Thank you for your time! Ben

griffithben commented 5 years ago

@casperbakker anymore thoughts on this?

griffithben commented 5 years ago

Any chance this can move forward in some fashion? If not, no worries.

casperbakker commented 4 years ago

I @griffithben,

I am sorry I left you in the dark about this. I really did not know what to do with this PR, and did not look at the PR's for some time because of that. That's my procrastination.

But I do appreciate your efforts and your kind attitude!

griffithben commented 4 years ago

@casperbakker No worries! Life gets busy so I understand. I ended up copying it over to https://github.com/brewerwall/php-barcode-generator/ so I could use it in another project.

I ended up running every combination of output and code types to get a baseline of images and files. Then I run tests against that as I refactor things to make sure I am not altering the output.

If you ever want to take a look and possibly merge it back into this repo one day, I would be more than happy to submit another PR!

casperbakker commented 4 years ago

@griffithben You made some nice improvements since the beginning, that is nice!

Finally came around to pushing my work in progress since 2016, which also was splitting at least all the types out to different files and add more test coverage. This make it easier to do bug fixes. My PR is here if you are interested: https://github.com/picqer/php-barcode-generator/pull/91

Maybe I will copy some of your improvements as well.

I am now trying to get the renderers and the types separated, so it is easier for users to make their own renderers.