Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
codemasher opened this issue Oct 25, 2023 · 11 comments
Open

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

codemasher opened this issue Oct 25, 2023 · 11 comments
Labels

Comments

@codemasher
Copy link
Member

codemasher commented Oct 25, 2023

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):

if($this->options->outputBase64){
$imageData = $this->toBase64DataURI($imageData, 'image/'.$this->options->outputType);
}

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):

$qrcode = new QRCode($options);
$qrcode->addByteSegment('https://github.com');
$qrOutputInterface = new QRImageWithLogo($options, $qrcode->getQRMatrix());
// dump the output, with an additional logo
// the logo could also be supplied via the options, see the svgWithLogo example
$out = $qrOutputInterface->dump(null, __DIR__.'/octocat.png');

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.

@codemasher codemasher added the bug label Oct 25, 2023
codemasher added a commit that referenced this issue Oct 25, 2023
@seebeen
Copy link

seebeen commented Jan 7, 2024

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
Copy link
Member Author

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
Copy link

seebeen commented Jan 16, 2024

I suppose the rework of the options object, and the API is out of the question? 😅

@codemasher
Copy link
Member Author

@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
Copy link
Member Author

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)

@seebeen
Copy link

seebeen commented Jan 19, 2024

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
Copy link
Member Author

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
Copy link

seebeen commented Jan 22, 2024

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

@codemasher
Copy link
Member Author

Semi-related: added basic intervention/image support in b231433

@devfisal
Copy link

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
Copy link
Member Author

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

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

No branches or pull requests

3 participants