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

Fix - Change default font for special letters #20

Merged
merged 3 commits into from
Mar 10, 2025

Conversation

RomainLvr
Copy link

@RomainLvr RomainLvr commented Mar 6, 2025

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Description

  • It fixes !36668

Screenshots (if appropriate):

image

@RomainLvr RomainLvr requested review from stonebuzz and Copilot March 6, 2025 14:27
@RomainLvr RomainLvr self-assigned this Mar 6, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@stonebuzz stonebuzz requested a review from Rom1-B March 6, 2025 17:11
@@ -78,7 +78,7 @@ public function __construct($format = 'A4', $orient = '')

$pdf->SetCreator('GLPI');
$pdf->SetAuthor('GLPI');
$font = 'helvetica';
$font = 'dejavusans';
Copy link

Choose a reason for hiding this comment

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

Aren't we just moving the problem to another language?

A few lines down (I can't comment on this), $font is overwritten by $_SESSION[‘glpipdffont’]. Where can we change this value? This is probably what's missing.

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the use of the glpipdffont session variable. Because apart from causing problems when applying fonts that don't take all character styles into account, it's not very useful.

Copy link

Choose a reason for hiding this comment

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

We have the same code in the core, so I assume that $_SESSION[‘glpipdffont’] was modifiable in the past. I think that's what we need to look into.

image

Copy link
Author

Choose a reason for hiding this comment

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

I've checked it's not modified anywhere in the code

Copy link

Choose a reason for hiding this comment

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

What I mean is that in the core and the plugin, we are trying to use $_SESSION['glpipdffont'], which is never defined.

The fix you're making in the plugin should also be applied in the core, I think.

Copy link
Author

Choose a reason for hiding this comment

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

okay 👍

@RomainLvr RomainLvr requested a review from Rom1-B March 7, 2025 10:33
@Rom1-B
Copy link

Rom1-B commented Mar 7, 2025

Please update the CHANGELOG and it will be OK for me.

@Rom1-B Rom1-B merged commit 0ffb2d1 into pluginsGLPI:main Mar 10, 2025
3 checks passed
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.

3 participants