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

Eccube/feature/fix php74 deprecated type declarations #6287

Draft
wants to merge 29 commits into
base: 4.3
Choose a base branch
from

Conversation

ci-wataru-kashii
Copy link
Contributor

@ci-wataru-kashii ci-wataru-kashii commented Sep 30, 2024

コマンド実行時、Webアプリケーションからのアクセス時に下記エラーを補足したため、修正しました。

Method "FQDN" might add "some_type" as a native return type declaration in the future. Do the same ...  "ECCUBEのFQDN" now to avoid errors or add an explicit @return annotation to suppress this message.

概要(Overview・Refs Issue)

PHP74_型宣言の追加.txt

方針(Policy)

EC-CUBE4.3 のシステム要件には、PHP8.1〜8.3 とのことですので、問題ない認識です。

https://www.php.net/manual/ja/language.types.declarations.php
より引用

関数のパラメータや戻り値、 クラスのプロパティ (PHP 7.4.0 以降)、クラス定数 (PHP 8.3.0 以降) に対して型を宣言することができます。

  • 親クラスの型宣言に合わせています。
  • mixed には、null も含まれるということで ?mixed はつけていません。

実装に関する補足(Appendix)

テスト(Test)

相談(Discussion)

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

…:offsetSet() , ArrayAccess::offsetUnset() , ArrayAccess::offsetUnset() メソッドの戻り値の型宣言 追加
…ctionNode::getSql() , Doctrine\ORM\Query\AST\Functions\FunctionNode::parse()
…nfigure() , Symfony\Component\Console\Command\Command::execute() , Symfony\Component\Console\Command\Command::initialize() , Symfony\Component\Console\Command\Command::interact()
…\DependencyInjection\Extension\Extension::getConfiguration()
…mponent\Form\DataTransformerInterface::reverseTransform() , Symfony\Component\Form\DataTransformerInterface::transform()
…ion\SessionInterface::clear() , Symfony\Component\HttpFoundation\Session\SessionInterface::registerBag() , Symfony\Component\HttpFoundation\Session\SessionInterface::replace() , Symfony\Component\HttpFoundation\Session\SessionInterface::save() , Symfony\Component\HttpFoundation\Session\SessionInterface::set() , Symfony\Component\HttpFoundation\Session\SessionInterface::setId() , Symfony\Component\HttpFoundation\Session\SessionInterface::setName()
…tor\DataCollector::reset() , Symfony\Component\HttpKernel\DataCollector\DataCollectorInterface::collect() , Symfony\Component\HttpKernel\DataCollector\DataCollectorInterface::getName()
…ent\Security\Core\User\UserInterface::eraseCredentials()
…Filters() , Twig\Extension\ExtensionInterface::getFunctions() , Twig\Extension\ExtensionInterface::getNodeVisitors()
…mplate::getSourceContext() , Twig\Template::getTemplateName()
@nanasess
Copy link
Contributor

EC-CUBE4.2互換のプラグインや、Customize クラスで、既存のクラスを継承している場合、互換性が壊れてしまうため、マイナーバージョンアップでの取り込みは難しそうですね😰

… , Symfony\Component\Form\AbstractType::finishView() , Symfony\Component\Form\AbstractType::getParent() , Symfony\Component\Form\AbstractTypeExtension::buildView()
@ci-wataru-kashii
Copy link
Contributor Author

@nanasess
コメントいただきありがとうございます!

EC-CUBE4.2互換のプラグインや、Customize クラスで、既存のクラスを継承している場合、互換性が壊れてしまうため、マイナーバージョンアップでの取り込みは難しそうですね😰

簡単ではありますが、下記検証をしてみたところ、特に問題なく見えますが

EC-CUBE4.2互換のプラグインや、Customize クラスで、既存のクラスを継承している場合、互換性が壊れてしまうため ところの認識が違うところがあれば、ご指摘いただけると幸いです。

  • パターン1)既存のEC-CUBE4.3に、EC-CUBE4.2互換のプラグインで既存のクラスを継承している場合

パターン1)既存のEC-CUBE4.3に、EC-CUBE4.2互換のプラグインで既存のクラスを継承している場合

▼EC-CUBE4.2 互換を想定したプラグイン作成

bin/console eccube:plugin:generate

EC-CUBE Plugin Generator Interactive Wizard
===========================================

 name [EC-CUBE Sample Plugin]:
 > Pattern1

 code [Sample]:
 > Pattern1

 ver [1.0.0]:
 >


 [OK] Plugin was successfully created: Pattern1 Pattern1 1.0.0

▼EC-CUBE4.2 互換を想定したプラグインで、管理画面>商品登録の入力フォーム拡張

  • 商品名を 5文字制限とした拡張
<?php
namespace Plugin\Pattern1\Form\Extension\Admin;

use Eccube\Common\EccubeConfig;
use Eccube\Form\Type\Admin\ProductType;
use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Validator\Constraints as Assert;

// 管理画面>商品管理>商品登録 - 拡張
class ProductExtension extends AbstractTypeExtension
{
    protected $eccubeConfig;
    public function __construct(EccubeConfig $eccubeConfig)
    {
        $this->eccubeConfig = $eccubeConfig;
    }

    public function buildForm(FormBuilderInterface $builder, array $options) // 戻り値の型宣言の追加なし
    {
        $builder
            ->add('name', TextType::class, [
                'constraints' => [
                    new Assert\NotBlank(),
                    new Assert\Length(['max' => 5]),    // 5文字制限
                ],
            ]);
    }

    /**
     * @see https://doc4.ec-cube.net/customize_formtype#%E3%82%B5%E3%83%B3%E3%83%97%E3%83%ABhttps://doc4.ec-cube.net/customize_formtype#%E3%82%B5%E3%83%B3%E3%83%97%E3%83%AB
     * {@inheritdoc}
     */
    public static function getExtendedTypes(): iterable
    {
        yield ProductType::class;
    }
}

▼入力エラー結果

入力エラー-商品登録画面

@ci-wataru-kashii
Copy link
Contributor Author

EC-CUBE4.2のシステム要件としては、PHP7.4〜8.1

型宣言については、関数のパラメータや戻り値、 クラスのプロパティ (PHP 7.4.0 以降)と記載があったのですが
私が認識していないところもあるかと思いますので、ご指摘に関する補足があると幸いです。
お忙しいところ、恐縮ですが、ご確認のほどよろしくおねがいします。

https://www.php.net/manual/ja/language.types.declarations.php

@nanasess
Copy link
Contributor

@ci-wataru-kashii

例えば、 EntryType をオーバーライドした CustomizeEntryType があるとします

<?php

namespace Customize\Form\Type\Front;

use Eccube\Common\EccubeConfig;
use Eccube\Form\Type\Front\EntryType;
use Symfony\Component\Form\FormBuilderInterface;

class CustomizeEntryType extends EntryType
{
    /**
     * @var EccubeConfig
     */
    protected $eccubeConfig;

    /**
     * EntryType constructor.
     *
     * @param EccubeConfig $eccubeConfig
     */
    public function __construct(EccubeConfig $eccubeConfig)
    {
        $this->eccubeConfig = $eccubeConfig;
    }

    /**
     * {@inheritdoc}
     */
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
    }
}

本PRを適用すると、以下のようなエラーになってしまいます。

PHP Fatal error:  Declaration of Customize\Form\Type\Front\CustomizeEntryType::buildForm(Symfony\Component\Form\FormBuilderInterface $builder, array $options) must be compatible with Eccube\Form\Type\Front\EntryType::buildForm(Symfony\Component\Form\FormBuilderInterface $builder, array $options): void in /var/www/html/app/Customize/Form/Type/Front/CustomizeEntryType.php

パターン1)既存のEC-CUBE4.3に、EC-CUBE4.2互換のプラグインで既存のクラスを継承している場合

上記が問題ないのは、 AbstractTypeExtension を継承しているためだと思われます。
EC-CUBE本体のクラスをオーバーライドした場合はエラーになってしまうかと。

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 88.70523% with 41 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (8dfeee5) to head (d95b3fa).
Report is 43 commits behind head on 4.3.

Files with missing lines Patch % Lines
src/Eccube/Session/Session.php 22.22% 7 Missing ⚠️
src/Eccube/Form/Type/Master/PageMaxType.php 0.00% 4 Missing ⚠️
...rc/Eccube/Form/Type/Admin/PluginManagementType.php 0.00% 3 Missing ⚠️
src/Eccube/Form/Type/Admin/SearchPluginApiType.php 0.00% 3 Missing ⚠️
src/Eccube/Form/Type/Admin/TagType.php 0.00% 3 Missing ⚠️
src/Eccube/Form/Type/ShoppingMultipleType.php 0.00% 3 Missing ⚠️
src/Eccube/Twig/Template.php 0.00% 3 Missing ⚠️
src/Eccube/Form/Type/Admin/PageType.php 0.00% 2 Missing ⚠️
.../Eccube/Form/Type/Admin/PluginLocalInstallType.php 0.00% 2 Missing ⚠️
src/Eccube/Form/Type/Admin/TwoFactorAuthType.php 0.00% 2 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##              4.3    #6287      +/-   ##
==========================================
- Coverage   82.69%   78.23%   -4.46%     
==========================================
  Files         480      480              
  Lines       26465    26527      +62     
==========================================
- Hits        21885    20754    -1131     
- Misses       4580     5773    +1193     
Flag Coverage Δ
E2E 78.23% <88.70%> (-4.46%) ⬇️
Unit 78.23% <88.70%> (-4.46%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@dotani1111 dotani1111 marked this pull request as draft November 14, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants