chillerlan / php-qrcode

A PHP QR Code generator and reader with a user-friendly API.
https://smiley.codes/qrcode/
Apache License 2.0
2.02k stars 302 forks source link

QRGdImage (custom): mime type is not properly set in base64 output #223

Open codemasher opened 1 year ago

codemasher commented 1 year ago

Issue

Currently there is an issue with custom output classes based on QRGdImage where the mime type of the base64 output is not properly set to the image type but rather to "custom". This happens when a custom output class uses the optional base64 encoding as it is implemented in the method QRGdImage::dump() and is invoked via the QROptions settings:

$options->outputType      = QROutputInterface::CUSTOM;
$options->outputInterface = MyCustomOutput::class; // extends QRGdImage

The "imageWithLogo" example is affected by this issue (among others):

https://github.com/chillerlan/php-qrcode/blob/57448af264a4bc0aca85a9ce61769aa6240b9481/examples/imageWithLogo.php#L66-L68

The resulting output would not properly display and looks similar to the following:

data:image/custom;base64,<encoded image data>

Further, this issue will always output the image as PNG as this is the default in the switch that determines the GD output function.

Affected versions

Affected by this issue are all versions of php-qrcode since optional base64 encoding was introduced: v3.x, v4.x and the upcoming v5.x.

Workaround

There are several workarounds available, one of which is already provided in the above example, that is, manual invocation of the output class and setting QROptions::$outputType to the desired output type (setting to "custom" is not necessary in this case and will be ignored):

https://github.com/chillerlan/php-qrcode/blob/57448af264a4bc0aca85a9ce61769aa6240b9481/examples/imageWithLogo.php#L99-L106

Another workaround could be to hardcode the output or mime type:

class MyCustomOutputextends QRGdImage{

    public function dump(string $file = null):string{
        // override the QROptions setting...
        $this->options->outputType = $this::GDIMAGE_PNG;

        // ...

        $imageData = $this->dumpImage();

        // ...or hard code the mime type
        if($this->options->outputBase64){
            $imageData = $this->toBase64DataURI($imageData, 'image/png');
        }

        return $imageData;
    }

}

Proposed fix

Since this is a low priority issue that can be easily worked around, v3.x and v4.x will not receive any fixes for it. For v5.x i will introduce separate classes for the several QRGdImage output types, such as QRGdImagePNG and so on. In v6, the class QRGdImage will be made abstract.

The base output class QROutputAbstract will see the addition of an internal constant MIME_TYPE that will set the type for the current class and which will be called in QROutputAbstract::toBase64DataURI().

Further, the setting QROptions::$outputType will be deprecated in favor of QROptions::$outputInterface, which in turn will also make calling custom classes easier.

seebeen commented 10 months ago

Hola. I've been working with your lib for one of my WP plugins Serbian Addons.

With a bit of reorganizing the options object and the output interface, all of the issues can be gone, and you can use only one QRGdImage class for all of the GD needs.

If that sounds good to you, I can extrapolate on my suggestion and make a small PoC. If you like it I can make a PR?

My universal implementation is here

https://github.com/oblakstudio/serbian-addons-for-woocommerce/blob/d383fdfa7deaf2573899c6d6bdc4c8c818545cdc/lib/QR/QR_Generator_GD.php#L75-L93

LMK what you think

codemasher commented 10 months ago

Hey, this is basically how it was/is implemented in v5 and earlier but it comes with several downsides such as testing issues and flexibility when extending - also match() is only available from PHP8+, so it's not suitable for v5. I reckon, having separate classes for the different GD output types is not ideal for other library providers, but usually if someone plans to implement a QR Code, they go for one type of output (that is PNG in most cases for quality reasons). Either way, you're not bound to using the extended classes - you can still extend the (now abstract) QRGdImage class and take it from there.

seebeen commented 10 months ago

I suppose the rework of the options object, and the API is out of the question? πŸ˜…

codemasher commented 10 months ago

@seebeen To be honest, I dislike most of the GD stuff and if it were up to me I'd remove it entirely in favor of ImageMagick and SVG haha. It has a lot of issues because the GD functions only accept integer values, which is one reason why the circular modules look weird on small scale (#23), among other things. The only reason i keep it is because GD is usually pre-installed unlike ImageMagick.

So if you have a meaningful proposal for a change that's less horrible than the current v5 or what's in dev-main, feel free to submit a PR.

codemasher commented 10 months ago

I should add:

seebeen commented 10 months ago

So if you have a meaningful proposal for a change that's less horrible than the current v5 or what's in dev-main, feel free to submit a PR.

I think more discussion is needed before PR submissions. πŸ˜…
Would you mind taking a look at intervention/image library, Oliver did some amazing stuff with driver detection. His lib automatically detects available libs and switches to the best one.

If you're open to making this lib work the same, a lot of the inner (and outer) workings can be abstracted and simplified.

codemasher commented 10 months ago

Hmm, i know this library but I don't see how it could solve my particular issue here and i don't want to add more layers of abstraction or more complicated driver detection (which isn't necessary as you choose the driver per output class). What i could do however is to introduce an output class that's based on intervention/image, similar to the FPDF class.

seebeen commented 10 months ago

I'll make a simple wrapper around your lib, and make a gist. See if you like it :)

codemasher commented 6 months ago

Semi-related: added basic intervention/image support in https://github.com/chillerlan/php-qrcode/commit/b231433d56512eb1fd31895e14e4f8c7c6a459fe

devfisal commented 6 months ago

I should add:

  • the $outputType option and its associated constants in QROutputInterface shall be gone
  • the output class is invoked and identified by its FQCN only
  • a valid mime type should be supplied for the base64 data URI (alternatively via finfo as in the Imagick output)

I just tried the example svgWithLogo.php. It would not work unless I added $options->outputType= QROutputInterface::CUSTOM;

So could you please indicate, how to handle this, if QROutputInterface shall be gone?

codemasher commented 6 months ago

You were running an example from dev-main (which already has the above fix implemented) with a release version, probably v5.x. Mix-and-match between major versions and the dev branch doesn't work, hence the note in bold text over here: https://github.com/chillerlan/php-qrcode?tab=readme-ov-file#documentation