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

Fixes: missing functionality for svg <use> elements #653

Closed
wants to merge 24 commits into from
Closed

Fixes: missing functionality for svg <use> elements #653

wants to merge 24 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 9, 2023

Fixes: #652

remicollet and others added 23 commits September 6, 2023 08:51
Tested and confirmed working in PHP 7.4 and PHP 8.2
* README: tone down the warning about tc-lib-pdf

Signed-off-by: Ruben Barkow-Kuder <github@r.z11.de>

* Update README.md

Co-authored-by: William Desportes <williamdes@wdes.fr>

---------

Signed-off-by: Ruben Barkow-Kuder <github@r.z11.de>
Co-authored-by: William Desportes <williamdes@wdes.fr>
* Fix of Deprecation warning with php 8.1 #614

* Update include/barcodes/qrcode.php

Co-authored-by: William Desportes <williamdes@wdes.fr>

---------

Co-authored-by: Robert Johnson Nallori <rnallori@evoketechnologies.com>
Co-authored-by: johnson361 <johnson361@users.noreply.github.com>
Co-authored-by: William Desportes <williamdes@wdes.fr>
* Update tcpdf_fonts.php

Fixes "use of "self" in callables is deprecated" warning is arising from tcpdf_fonts.php when using PHP >= 8.2

* Update tcpdf_fonts.php for PHP 5.3-8.2 compatibility

PHP 8.2 "use of "self" in callables is deprecated" yet some ways of fixing this breaks for PHP 5.3. This approach works, tested PHP 5.3.29 - 8.2.0

* Update tcpdf_fonts.php

Spaces added back in before arguments

* Update tcpdf_fonts.php using get_called_class()

Maneuvers compatibility of callables inside array_map() between PHP 5.3 and 8.2 - tested.

---------

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
* Fix composite glyph output

* Pad zeros before checksum calulation

---------

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
* fix for #583

* fix fix

---------

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
* Fix non-numeric value warning

Fixes this warning on generating PDF:
Warning: A non-numeric value encountered in /tcpdf/tcpdf.php on line 5473

* Better fix for non-numeric value warning

Fixes this warning on generating PDF after calling `Text` with a non-numeric value for `$fstroke`:
Warning: A non-numeric value encountered in /tcpdf/tcpdf.php on line 5470

* Update tcpdf.php

Co-authored-by: William Desportes <williamdes@wdes.fr>

---------

Co-authored-by: William Desportes <williamdes@wdes.fr>
Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
This was already fixed in tc-lib-barcode.
* Fix return type annotation

* BC with tools that do not support PHPStan annotations

Co-authored-by: William Desportes <williamdes@wdes.fr>

---------

Co-authored-by: William Desportes <williamdes@wdes.fr>
Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
#598)

* Typehints for get/setHeaderMargin are inconstent

* Add typehints for header/footer margin properties

---------

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
* Bump actions/checkout and add PHP 8.3

* Update the composer json tests

* Remove PHP 5.3 and 5.4 from the matrix

* add permission entry

Restrict GitHub actions access

---------

Co-authored-by: Nicola Asuni <nicolaasuni@users.noreply.github.com>
@CLAassistant
Copy link

CLAassistant commented Oct 9, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
16 out of 17 committers have signed the CLA.

✅ johnson361
✅ kosmro
✅ rubo77
✅ bruno-farias
✅ ren1244
✅ littlepackage
✅ theonlytruth
✅ remicollet
✅ d-salerno
✅ simonbuehler
✅ mvorisek
✅ nicolaasuni
✅ williamdes
✅ fisharebest
✅ plsnk
✅ survik1
❌ Nicole Krämer


Nicole Krämer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

tcpdf.php Outdated Show resolved Hide resolved
@@ -1641,6 +1641,20 @@ class TCPDF {
*/
protected $svgdefs = array();

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid this patch is not very minimal and so will have less changes being accepted into tcpdf

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library is part of the moodle core. We at TU Graz are relying on it to generate certificates, which among other contents contain logos of our partners (SVGs). Its crucial for us that they are correct on the pdf and currently this is not the case. Hence our contribution to this library.
Is there anything that can be done to increase chances? I will change up the contribution in any way necessary to provide this functionality. (:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, thank you for contributing to the libraries you use !

Second, I am here as an advisor and try to monitor issues and pull-requests on my free time
Currently the library owner and maintainer remains mysterious in choices that are made. But it's mostly the rule of more the change is okay and small more it has chances to get on a release one day.

So from time to time I build an issue with a summary of PRs: #634
Maybe it helps, I never know, nobody knows
So it's a wait and see, but you need to get it slim and clean and right. Or it will probably get refused and you are back in a 6-8 months loop to get feedback on the new changes

This recent PR changed the library maintenance official statement, and that's a relief: #589

In summary, I think this patch will appear too large. Is there other ways to change less code ?

implement review changes

Co-authored-by: William Desportes <williamdes@wdes.fr>
Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for making this patch !

@BsAtHome
Copy link

Hi,
There are still a few problems with this patch applied. The <use> element is more complicated than implemented by this patch. I have a problem file that shows the issue(s) (a cooked down test version from the original shown below).
There are three clipPath scenarios that I tested.

  • First is <use> in the <defs><clipPath> section with a forward reference to a path later on in the document. This scenario fails completely and no clipPath is applied.
  • Second is a copied version of the document's <path> in the <defs><clipPath> section, but it retained the original transformation attribute from the source. This scenario failed to apply the transformation.
  • Third is a working version, where the path is normalized.

The used svg (inkscape displays all three version correct):
<svg id="testUseClippath" width="100%" height="100%" viewBox="0 0 50 30" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<defs>
<clipPath id="usepath"><use xlink:href="#realpath" /></clipPath>
<!-- <clipPath id="usepath"><path id="realpath" d="M 14,-1 C 10,1 6,9 0,9 C -6,9 -12,1 -14,-1 C -11,-2 -6,-9 0,-9 C 6,-9 11,-2 14,-1 Z" transform="translate(25,15)" /></clipPath> -->
<!-- <clipPath id="usepath"><path d="M 39,14 C 35,16 31,24 25,24 C 19,24 13,16 11,14 C 14,13 19,6 25,6 C 31,6 36,13 39,14 Z" /></clipPath> -->
</defs>
<g>
<rect x="0.1" y="0.1" width="49.8" height="29.8" style="fill:none;stroke:#cc0;stroke-width:0.2" />
<g clip-path="url(#usepath)">
<rect x="0" y="0" width="50" height="30" style="fill:#222;stroke:none" />
<ellipse cx="25" cy="25" rx="15" ry="10" style="fill:#800f08;stroke:none" />
</g>
<path id="realpath" d="M 14,-1 C 10,1 6,9 0,9 C -6,9 -12,1 -14,-1 C -11,-2 -6,-9 0,-9 C 6,-9 11,-2 14,-1 Z" transform="translate(25,15)" style="fill:none;stroke:black;stroke-width:1" />
</g>
</svg>

@nicolaasuni
Copy link
Member

Please try to resolve the conflicts.

@ghost ghost closed this by deleting the head repository Oct 4, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing functionality for svg <use> elements