eps() and svg() behaves differently from png()

[matslindh]

The eps() method and the svg() function both return the content generated, instead of outputting it (which png() does), even though the documentation says otherwise. The methods also set the headers for the content they're outputting, so it seems it wasn't meant to just return the data.

I have a patch that changes this, but as there is a BC concern I'd like to discuss whether we actually want to do that. The patch changes the return to a print / echo, which brings behavior in line with png(), but will break applications that retrieves the content and then stores it somewhere else instead. We might handle that by introducing a second method (I'd prefer to implement the different behaviors as different methods anyway, really) that replicates the old functionality, or introduce the "correct" behavior as a new method - this will require updating the documentation and might be confusing later.

For vectors: https://github.com/t0k4rt/phpqrcode/blob/master/qrvect.php#L37

For images: https://github.com/t0k4rt/phpqrcode/blob/master/qrimage.php#L36

[steinsag]

In general, I think this library is trying to do too much. It would be great to reduce the scope of the library to only generate QR codes in different output formats and return that. It is up to the user of the library to decide whether he wants to send the QR code directly to the browser, save it to disk, store it in a database, transform it to a base64 string to include it as inline image in CSS, etc. Currently, the library does too many things in too many different ways for each format and would really benefit from some serious refactoring and also some test automation.

Unfortunately, it seems that @t0k4rt is not able to continue development. Any ideas how to proceed?

[t0k4rt]

Hi guys, actually I agree with you. First of all, yes the lib is complicated and do lots of stuff we barely use but this is a legacy from the lib we originally picked up. At that time, this lib was only lacking vector support which I added. Secondly yes it needs refactoring, and tests. In my opinion, after some time using it, and according to what I see now about qr codes, this lib should be simpler, this lib should only generate a binary array depending on content and error correction, then another lib could take care of color management (rgb, cmyk) and shape effects and lastly one lib could take care of format conversion. But it's really an opinion. Finally, I've got not enough time to deal with the lib, I'll do my best to review the PR you sent to me. The lib is used on some projects and it might be a problem to break BC. This is my main concern with this patch.

[matslindh]

The CMYK generation can go in without breaking BC, so that should be good to go anyway.

The change between return / echo is a BC break, and will leave applications that have worked around this bug with invalid output (if they're already outputting the returned value). We can leave that one out for now.

The next step would probably be to rework the output code into separate handlers, one for each format - so that they can be configured separately and dropped in from a 3rd party source if needed. I don't think color / shape management needs its own module structure at the moment, as the output modules probably would need to know how to handle that anyway.

Depending on the license for phpqrlib we might be able to rework the interesting parts into a more pluggable / independent architecture, but for all the kludges of the library, it works great and is able to generate loads of QR-codes in different formats effectively.

[steinsag]

Just created a pull request to turn library into a composer artifact and also to enable unit testing via phpunit.

[pcr-coding]

Are there any ongoing plans on refactoring this library?

I really like this library but working with the code is a mess and it really would benefit from a refactoring. The project would also benefit from some standards like PSR-2 and PSR-4 and some proper docblocking. What I'm thinking about is a core composer package doing all the basic calculating stuff and some more packages doing the output stuff (each for one format). So that it is easily extendable with further formats and we only need to install svg support if we don't need png and eps for example.

I even would go as far and consider breaking BC for a better and more modular code. Maybe it could be an option to provide a compatibility package to make it BC for them who need it.

Are there some people interested in discussing a plan how refactoring could proceed?