Moodle

Removing fpdf from core since it's not used by anothing in core

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Duplicate
  • Affects Version/s: 1.6.8, 1.7.6, 1.8.7, 1.9.3
  • Fix Version/s: 2.0
  • Component/s: Libraries
  • Labels:
    None

Description

FPDF doesn't appear to be used anywhere in core Moodle.

However, because it's shipped with the core Moodle, that means that we are responsible for any security problems it may have in the future. I suggest that we remove it from core to reduce the risk of having to issue a Moodle security advisory just for that.

Francois

Issue Links

Activity

Hide
Dan Marsden added a comment -

my +1 to remove FPDF - afaik, it doesn't support UTF8 - but TCPDF does.

from what I can see, DFwiki and the certificate module both use fpdf - it would be trivial to remove fpdf from certificate module as it uses both anyway, not sure on dfwiki or other contrib modules.

it should be really trivial to remove FPDF too shouldn't it? - I don't even think there's a core wrapper for FPDF - just one for TCPDF

Show
Dan Marsden added a comment - my +1 to remove FPDF - afaik, it doesn't support UTF8 - but TCPDF does. from what I can see, DFwiki and the certificate module both use fpdf - it would be trivial to remove fpdf from certificate module as it uses both anyway, not sure on dfwiki or other contrib modules. it should be really trivial to remove FPDF too shouldn't it? - I don't even think there's a core wrapper for FPDF - just one for TCPDF
Hide
Eloy Lafuente (stronk7) added a comment -

+1 to suppress unused code but with some considerations:

1) Warn and advise everywhere about that library being dropped in XX days (to give people time to upgrade to tcpdf / embed fpdf into their solutions). Also, perhaps we could use this wrapper: CONTRIB-622
2) Have one doubt about why tcpdf won't be exposed to the same (security) problems than fpdf when it seems to be based on it.
3) Also, there is one problem about the full size of current tcpdf library (fonts). See MDL-16802

Ciao

Show
Eloy Lafuente (stronk7) added a comment - +1 to suppress unused code but with some considerations: 1) Warn and advise everywhere about that library being dropped in XX days (to give people time to upgrade to tcpdf / embed fpdf into their solutions). Also, perhaps we could use this wrapper: CONTRIB-622 2) Have one doubt about why tcpdf won't be exposed to the same (security) problems than fpdf when it seems to be based on it. 3) Also, there is one problem about the full size of current tcpdf library (fonts). See MDL-16802 Ciao
Hide
Dan Poltawski added a comment -

+1 too

Perhaps now we are breaking things for third party modules in 2.0 it would be a good time to audit the current libraries to find any other unused ones?

Show
Dan Poltawski added a comment - +1 too Perhaps now we are breaking things for third party modules in 2.0 it would be a good time to audit the current libraries to find any other unused ones?
Hide
Eloy Lafuente (stronk7) added a comment -

Adding some watchers and assigning to MD

Show
Eloy Lafuente (stronk7) added a comment - Adding some watchers and assigning to MD
Hide
Martin Dougiamas added a comment -

Having a PDF library in Moodle is a good idea:

1) it's used by the popular certificate module and I think some other 3rd party modules (Book?)
2) the portfolio API in Moodle 2.0 will need it

It could probably use some trimming down (fonts), but we do need to have one.

I'm unsure of the relationship between fpdf and tcpdf ... if we can lose fpdf and keep tcpdf working then my +1 for that!

Show
Martin Dougiamas added a comment - Having a PDF library in Moodle is a good idea: 1) it's used by the popular certificate module and I think some other 3rd party modules (Book?) 2) the portfolio API in Moodle 2.0 will need it It could probably use some trimming down (fonts), but we do need to have one. I'm unsure of the relationship between fpdf and tcpdf ... if we can lose fpdf and keep tcpdf working then my +1 for that!
Hide
David Mudrak added a comment -

Here is the summary of my fpdf/tcpdf usage experience (small certificate customization, course ordering and invoicing module):

AFAIK tcpdf is the successor of fpdf as the API is almost identical. Upgrading products ti use tcpdf instead of fpdf is not hard task. Plus, tcpdf has UTF8 support. Without UTF8 support, fpdf is useless for non-English Moodle users.

Regarding fonts - we can customize tcpdf a bit to load fonts from moodledata (pretty easy configuration change) and offer them as an optional pack at download.moodle.org. Then we can ship the core tcpdf with just one default font - I propose Deja Vu for its well known UTF8 support.

There are some small issues with tcpdf configuration left - eg. cache paths and thier URLs / that should be reviwed from the security point of view.

I am offering myself as a maintainer of tcpdf lib in the core.

Show
David Mudrak added a comment - Here is the summary of my fpdf/tcpdf usage experience (small certificate customization, course ordering and invoicing module): AFAIK tcpdf is the successor of fpdf as the API is almost identical. Upgrading products ti use tcpdf instead of fpdf is not hard task. Plus, tcpdf has UTF8 support. Without UTF8 support, fpdf is useless for non-English Moodle users. Regarding fonts - we can customize tcpdf a bit to load fonts from moodledata (pretty easy configuration change) and offer them as an optional pack at download.moodle.org. Then we can ship the core tcpdf with just one default font - I propose Deja Vu for its well known UTF8 support. There are some small issues with tcpdf configuration left - eg. cache paths and thier URLs / that should be reviwed from the security point of view. I am offering myself as a maintainer of tcpdf lib in the core.
Hide
Dan Poltawski added a comment -

One thing we noticed which might be barrier to moving wholesale to tcpdf is that there is an additional file in the fpdf directory: fpdfprotection.php

Which looks like it was added with this bug: MDL-5855

Show
Dan Poltawski added a comment - One thing we noticed which might be barrier to moving wholesale to tcpdf is that there is an additional file in the fpdf directory: fpdfprotection.php Which looks like it was added with this bug: MDL-5855
Hide
David Mudrak added a comment -

Locking features implemented in fpdfprotection.php are implemented in the standard tcpdf - see tcpdf::SetProtection()

Show
David Mudrak added a comment - Locking features implemented in fpdfprotection.php are implemented in the standard tcpdf - see tcpdf::SetProtection()
Hide
Chardelle Busch added a comment - - edited

Here was my thinking on the pdf libs for the certificate:

I left fpdf in there to be used with langs that will work with it, e.g. english and other langs that don't use characters. I believe it works with most symbols (accents, etc.) too because of the added utf8_decode. Why? Because tcpdf embeds the entire font into the pdf, making for very large files (can be several megs depending on the size of any images used). This is why only one font is used in the unicode certificate types. This can be a problem especially if users are saving the certificate files in the course files and may have limited space, and could be a problem with some email programs if the certificate is mailed.

Tcpdf was added to be used by langs than won't work with fpdf, e.g. langs with characters. Those two unicode certificate types use only the Freesans and Freesans Bold fonts. I do know that some chinese languages need to create a new font for it to work using the make font utility. I have no idea what the "dejavu" fonts are--I didn't put those in there. Last I looked, there was no way to get around this, but there may be? I believe that Joomla switched from fpdf to tcpdf with 1.5. I might be worth taking a look at how they are using it.

I added the "protection" script to the fpdf lib, the tcpdf code already includes a protection script, so that isn't a problem.

And yes, tcpdf was built upon fpdf to use utf8 and is maintained by someone else--it is not really a successor.

Bottom line – if you switch over exclusively to tcpdf, certificates using more than one font (which I think can make for a very nice certificate) and large images could conceivably be up to 5 or 6 megs in size (if I remember, those with just one font were about 3 megs). So, if this is a problem for users, perhaps fpdf should still be an option (addon, etc.).

Hope this helps.

Show
Chardelle Busch added a comment - - edited Here was my thinking on the pdf libs for the certificate: I left fpdf in there to be used with langs that will work with it, e.g. english and other langs that don't use characters. I believe it works with most symbols (accents, etc.) too because of the added utf8_decode. Why? Because tcpdf embeds the entire font into the pdf, making for very large files (can be several megs depending on the size of any images used). This is why only one font is used in the unicode certificate types. This can be a problem especially if users are saving the certificate files in the course files and may have limited space, and could be a problem with some email programs if the certificate is mailed. Tcpdf was added to be used by langs than won't work with fpdf, e.g. langs with characters. Those two unicode certificate types use only the Freesans and Freesans Bold fonts. I do know that some chinese languages need to create a new font for it to work using the make font utility. I have no idea what the "dejavu" fonts are--I didn't put those in there. Last I looked, there was no way to get around this, but there may be? I believe that Joomla switched from fpdf to tcpdf with 1.5. I might be worth taking a look at how they are using it. I added the "protection" script to the fpdf lib, the tcpdf code already includes a protection script, so that isn't a problem. And yes, tcpdf was built upon fpdf to use utf8 and is maintained by someone else--it is not really a successor. Bottom line – if you switch over exclusively to tcpdf, certificates using more than one font (which I think can make for a very nice certificate) and large images could conceivably be up to 5 or 6 megs in size (if I remember, those with just one font were about 3 megs). So, if this is a problem for users, perhaps fpdf should still be an option (addon, etc.). Hope this helps.
Hide
Dan Marsden added a comment -

afaik tcpdf allows you to choose not to embed fonts for non-utf8 docs - by using MakeFont and passing false as the 3rd param. So tcpdf doesn't need to "force" embedded fonts. Haven't tried this, but it looks like it is supported.

Show
Dan Marsden added a comment - afaik tcpdf allows you to choose not to embed fonts for non-utf8 docs - by using MakeFont and passing false as the 3rd param. So tcpdf doesn't need to "force" embedded fonts. Haven't tried this, but it looks like it is supported.
Hide
Chardelle Busch added a comment -

I am going to begin testing of tcpdf version 4.6.015 (updated today: 2009-06-11) that requires PHP 5 for Moodle 2.0. It allows for non-embedded fonts. My suggestion is to customize this version for our requirements and then drop fpdf lib as of 2.0.

My To Do:

1. Test current usage (e.g. config, protection script, etc.) of latest version in 2.0.
2. Create and test non-embedded fonts.

We need to make a decision on fonts--which ones to include by default and where (if) other fonts can be downloaded.

Show
Chardelle Busch added a comment - I am going to begin testing of tcpdf version 4.6.015 (updated today: 2009-06-11) that requires PHP 5 for Moodle 2.0. It allows for non-embedded fonts. My suggestion is to customize this version for our requirements and then drop fpdf lib as of 2.0. My To Do: 1. Test current usage (e.g. config, protection script, etc.) of latest version in 2.0. 2. Create and test non-embedded fonts. We need to make a decision on fonts--which ones to include by default and where (if) other fonts can be downloaded.
Hide
Chardelle Busch added a comment -

See CONTRIB-622

I've got the certificate for 2.0 using only tcpdf via pdflib.php (I added the config variables there)

The latest version includes the protection script and non-embedded fonts. So the tcpdf folder requires no hacks or additional files

Show
Chardelle Busch added a comment - See CONTRIB-622 I've got the certificate for 2.0 using only tcpdf via pdflib.php (I added the config variables there) The latest version includes the protection script and non-embedded fonts. So the tcpdf folder requires no hacks or additional files
Hide
Chardelle Busch added a comment -

I do not have permission to commit the attached pdf.lib file, if someone could please do that for me, then I can update the tcpdf folder to the latest version deleting all unnecessary fonts, flies, etc.

I have added the config to this file, so no hacks need be made to the tcpdf.php file.

Also, please delete the fpdf folder.

Show
Chardelle Busch added a comment - I do not have permission to commit the attached pdf.lib file, if someone could please do that for me, then I can update the tcpdf folder to the latest version deleting all unnecessary fonts, flies, etc. I have added the config to this file, so no hacks need be made to the tcpdf.php file. Also, please delete the fpdf folder.
Hide
David Mudrak added a comment -

Chardelle, thanks for your work on this. Have you got a proposal regarding the fonts issue? This is a copy&paste from a Moodle developers chatroom:

(20:09:12) david: I am going to spend a day with TCPDF fonts issus to make them downloadable so TCPDF does not take so many megs
(20:10:01) Eloy: download to moodledata using componentslib (like langs) sounds great IMO
(20:10:17) david: yes, that is the planned future
(20:11:09) Eloy:
(20:12:11) Petr ?koda: yep, good idea
(20:14:23) Eloy: not critical though
(20:15:30) david: critical step one is to prepare a stripped version of recent tcpdf to replace the current one. Then making fonts downloadable.
(20:16:23) Eloy: chardelle posted one somewhere (recently)... isn't it?
(20:16:58) david: Yes. And I have prepared one some time ago, too
(20:17:07) david: I am in contact with tcpdf author

I have a customized version of Certificate working with tcpdf nicely in 1.8, too.

Also, I was thinking about a script that produces a PDF containing alphabetical characters from all installed language packs (we have an alphabet as a string in every language pack). That might be a good test and the hint for the default font to use. Still, my +1 for Deja Vu.

Show
David Mudrak added a comment - Chardelle, thanks for your work on this. Have you got a proposal regarding the fonts issue? This is a copy&paste from a Moodle developers chatroom: (20:09:12) david: I am going to spend a day with TCPDF fonts issus to make them downloadable so TCPDF does not take so many megs (20:10:01) Eloy: download to moodledata using componentslib (like langs) sounds great IMO (20:10:17) david: yes, that is the planned future (20:11:09) Eloy: (20:12:11) Petr ?koda: yep, good idea (20:14:23) Eloy: not critical though (20:15:30) david: critical step one is to prepare a stripped version of recent tcpdf to replace the current one. Then making fonts downloadable. (20:16:23) Eloy: chardelle posted one somewhere (recently)... isn't it? (20:16:58) david: Yes. And I have prepared one some time ago, too (20:17:07) david: I am in contact with tcpdf author I have a customized version of Certificate working with tcpdf nicely in 1.8, too. Also, I was thinking about a script that produces a PDF containing alphabetical characters from all installed language packs (we have an alphabet as a string in every language pack). That might be a good test and the hint for the default font to use. Still, my +1 for Deja Vu.
Hide
David Mudrak added a comment -

Chardelle, please wait a day or two. I will incorporate your changes into MDL-15055 and will commit.

I have managed to strip the size of tcpdf 4.6 from 17MB to 7.5MB. More 300KB will be saved by removing obsolete lib/fpdf/

I went thru TCPDF doc and decided to keep just their core fonts and DejaVu fonts. DejaVu is well known for it huge language coverage which is something we need in Moodle.

Let me assign you as a tester of these changes. Thanks in advance.

Show
David Mudrak added a comment - Chardelle, please wait a day or two. I will incorporate your changes into MDL-15055 and will commit. I have managed to strip the size of tcpdf 4.6 from 17MB to 7.5MB. More 300KB will be saved by removing obsolete lib/fpdf/ I went thru TCPDF doc and decided to keep just their core fonts and DejaVu fonts. DejaVu is well known for it huge language coverage which is something we need in Moodle. Let me assign you as a tester of these changes. Thanks in advance.
Hide
Chardelle Busch added a comment - - edited

I went with freesans (and freeserif) since it was used as default font in the pdflib file. But whatever, just let me know. Even though it will take up a couple more megs, I do like have two choices for the default font (both the sans and the serif) – it just looks nice in the certificate, but the serif is certainly optional.

Here's my suggestion for the tcpdf folder:

leave in empty cache folder--just in case
leave in config folder--no changes, can delete lang ita file
delete doc folder
delete examples folder
font folder--leave all font files without zips (non-embedded), e.g. courier, etc., delete all fonts with zips (embedded) except default (both sans and serif), delete utils folder
images folder--leave only blank.png
delete barcodes files

This is just a little over 6MB on my machine with the freesans and freeserif fonts with the latest tcpdf.

One question for you--if we are going to make fonts installable (e.g. I am assuming it will work similarly to the langs), then do we want to take the fonts folder out of the lib/tcpdf folder and put it somewhere else, e.g. lang/fonts?

Show
Chardelle Busch added a comment - - edited I went with freesans (and freeserif) since it was used as default font in the pdflib file. But whatever, just let me know. Even though it will take up a couple more megs, I do like have two choices for the default font (both the sans and the serif) – it just looks nice in the certificate, but the serif is certainly optional. Here's my suggestion for the tcpdf folder: leave in empty cache folder--just in case leave in config folder--no changes, can delete lang ita file delete doc folder delete examples folder font folder--leave all font files without zips (non-embedded), e.g. courier, etc., delete all fonts with zips (embedded) except default (both sans and serif), delete utils folder images folder--leave only blank.png delete barcodes files This is just a little over 6MB on my machine with the freesans and freeserif fonts with the latest tcpdf. One question for you--if we are going to make fonts installable (e.g. I am assuming it will work similarly to the langs), then do we want to take the fonts folder out of the lib/tcpdf folder and put it somewhere else, e.g. lang/fonts?
Hide
David Mudrak added a comment -

Chardelle - you are right. FreeFont will be a better choice than DejaVu. As my PDF test page shows, FreeFont actually seems to support more Moodle languages than DejaVu does. And as you have tested it, let us stay with it. I will keep both FreeSans and FreeSerif.

This is how I have modified /lib/tcpdf so far:
cache/ - removed, we are using Moodle cache directory and web servers should not be able to write anywhere under $CFG->dirroot, anyway
config/ - removed lang/ (Moodle localization will be used) and alternative configuration
doc/ and examples/ removed
font/ all "core fonts" (non-embedded) kept. Utils deleted. freesans* and freeserif* kept
images/ removed, using Moodle's blank image
barcodes files kept - just in case we want to add barcodes on Certificate or so

Fonts are going to be installed into $CFG->dataroot/fonts/. If such folder exists, it is used instead of standard lib/tcpdf/fonts/. Actually, this has been supported for some time already - since Vy-Shane created lib/pdflib.php wrapper.

I am going to commit soon - will let you know.

Show
David Mudrak added a comment - Chardelle - you are right. FreeFont will be a better choice than DejaVu. As my PDF test page shows, FreeFont actually seems to support more Moodle languages than DejaVu does. And as you have tested it, let us stay with it. I will keep both FreeSans and FreeSerif. This is how I have modified /lib/tcpdf so far: cache/ - removed, we are using Moodle cache directory and web servers should not be able to write anywhere under $CFG->dirroot, anyway config/ - removed lang/ (Moodle localization will be used) and alternative configuration doc/ and examples/ removed font/ all "core fonts" (non-embedded) kept. Utils deleted. freesans* and freeserif* kept images/ removed, using Moodle's blank image barcodes files kept - just in case we want to add barcodes on Certificate or so Fonts are going to be installed into $CFG->dataroot/fonts/. If such folder exists, it is used instead of standard lib/tcpdf/fonts/. Actually, this has been supported for some time already - since Vy-Shane created lib/pdflib.php wrapper. I am going to commit soon - will let you know.
Hide
David Mudrak added a comment -

This has been fixed by MDL-18664.

Show
David Mudrak added a comment - This has been fixed by MDL-18664.
Hide
Chardelle Busch added a comment -

Great David. Everything seems to be working fine. The only thing I noticed is that the pdflib file is missing its final ?>

Show
Chardelle Busch added a comment - Great David. Everything seems to be working fine. The only thing I noticed is that the pdflib file is missing its final ?>
Hide
David Mudrak added a comment -

Chardelle - that is intentional. According to Moodle Coding style [1], we do not use the closing PHP tags any more. There are good reasons for it. Thanks for the review. Please close this issue if you do not spot any problems.

[1] http://docs.moodle.org/en/Development:Coding_style#PHP_tags

Show
David Mudrak added a comment - Chardelle - that is intentional. According to Moodle Coding style [1], we do not use the closing PHP tags any more. There are good reasons for it. Thanks for the review. Please close this issue if you do not spot any problems. [1] http://docs.moodle.org/en/Development:Coding_style#PHP_tags

Dates

  • Created:
    Updated:
    Resolved: