Added PDF font switcher #46
Conversation
Kana/etc/fieldset.xml
Outdated
| @@ -47,9 +47,11 @@ | |||
| <fieldset id="sales_convert_quote_address"> | |||
| <field name="firstnamekana"> | |||
| <aspect name="to_order_address" /> | |||
| <aspect name="to_customer_address" /> | |||
There was a problem hiding this comment.
Change set for this file seems to be out of scope for this Pull Request.
Pdf/Helper/Data.php
Outdated
| /** | ||
| * @var string | ||
| */ | ||
| protected $_dir; |
There was a problem hiding this comment.
Current edition of Magento Technical Guidelines suggests to use composition over inheritance, therefore it may be better to change modifier to private.
Pdf/Helper/Data.php
Outdated
| * Class Data | ||
| * @package MagentoJapan\Pdf\Helper | ||
| */ | ||
| class Data extends \Magento\Framework\App\Helper\AbstractHelper |
There was a problem hiding this comment.
Having Helper classes is not required and sometimes may even cause confusion for 3rd party developers to figure out the purpose of the class. I would suggest moving configuration reading logic to a separate model, with clear name reflecting it's purpose.
There was a problem hiding this comment.
Agree. @HirokazuNishi, to convert this class from helper to service class you may rename it to MagentoJapan\Pdf\ModelConfig and inject only scope config and dir reader as dependencies.
Pdf/Helper/Data.php
Outdated
| /** | ||
| * font file path | ||
| */ | ||
| const FONT_DIR = 'lib/fonts/'; |
There was a problem hiding this comment.
This constant might as well be made configurable via di.xml files.
Pdf/Helper/Data.php
Outdated
| /** | ||
| * @var string | ||
| */ | ||
| protected $_dir; |
There was a problem hiding this comment.
Adding underscore to private properties is more or less outdates convention. It's safe to use names without it.
| /** | ||
| * font file path | ||
| */ | ||
| const FONT_DIR = 'lib/fonts/'; |
There was a problem hiding this comment.
There is already a similar constant used in the Helper. It would make sense to make it configurable from a single point.
Pdf/Model/Order.php
Outdated
| protected function _setFontItalic($object, $size = 7) | ||
| { | ||
| if ($this->getHelper()->getJapaneseFontIsActive()) { | ||
| $fontpath = $this->getHelper()->getJapaneseFont(); |
There was a problem hiding this comment.
This part (following 3 lines) seems to be shared across multiple classes and methods. It makes sense to extract it.
Pdf/Test/Unit/Helper/DataTest.php
Outdated
| * Class DataTest | ||
| * @package MagentoJapan\Pdf\Test\Unit\Helper | ||
| */ | ||
| class DataTest extends \PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
Unfortunately PHPUnit version used by Magento has been updated and this would not be compatible anymore with 2.2 and 2.3
|
Thank you for the PR. I've done a brief code review. Please feel free to take a look at the comments above. If you don't mind I would like to collaborate and push some of the fixes to your branch as well. Thanks! |
Pdf/Helper/Data.php
Outdated
| * Class Data | ||
| * @package MagentoJapan\Pdf\Helper | ||
| */ | ||
| class Data extends \Magento\Framework\App\Helper\AbstractHelper |
There was a problem hiding this comment.
Agree. @HirokazuNishi, to convert this class from helper to service class you may rename it to MagentoJapan\Pdf\ModelConfig and inject only scope config and dir reader as dependencies.
| \Magento\Framework\Module\Dir\Reader $dirReader | ||
| ) | ||
| { | ||
| $this->_dir = $dirReader->getModuleDir('', 'MagentoJapan_Pdf') . '/' . self::FONT_DIR; |
There was a problem hiding this comment.
Inside constructor please only assign dependencies to private properties if something should be calculated it should be done when some method called
Pdf/Model/Creditmemo.php
Outdated
| */ | ||
| protected function _setFontRegular($object, $size = 7) | ||
| { | ||
| if ($this->getHelper()->getJapaneseFontIsActive()) { |
There was a problem hiding this comment.
Such approach creates a lot of code duplication and if some 3rd party module has own pdf generated documents it will not be compatible.
We may try to create after plugin on Magento\Framework\Filesystem\Directory\ReadInterface::getAbsolutePath which will substitute result fort particular font paths. As we may limit check by in_array function call I believe there should not be dramatical performance degradation.
There was a problem hiding this comment.
I tried to set plugin against Magento\Framework\Filesystem\Directory\ReadInterface, but Magento\Framework\Filesystem\Directory\ReadFactory has hardcoded class name for generating Read class instance. Seems it's impossible to set plugin against ReadInterface.
|
IPA font is distributed under IPA font license. |
add IPA Font License text
|
Static test failures regarding |
Added PDF font switcher module for Japanese. It can generate Invoice, Shipment, and Creditmemo with embedded Japanese font.