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

テーマをコピーした際にヘルパーの重複エラーが表示される #3978 #4102

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

IwasakiRyuichi
Copy link
Contributor

@ryuring
大変お待たせしました。コピーしたテーマが正常にフロント画面に表示されるようになりましたのでご確認よろしくお願いします。

@ryuring
Copy link
Collaborator

ryuring commented Dec 29, 2024

@IwasakiRyuichi changePluginNameSpace や、changePluginClassName と処理がほぼ一緒ですので統合できないか一度考えてみてください。

@ryuring ryuring added the Reviewed レビュー済 label Dec 29, 2024
@IwasakiRyuichi
Copy link
Contributor Author

@ryuring
修正完了し、動作も確認できましたが修正内容こちらでよろしいでしょうか?

修正内容
「プラグインのnamespaceを変更する処理とヘルパーのnamespaceを変更する処理」
「プラグインのクラス名とヘルパーのクラス名を変更する処理」
この2つを1つの関数にまとめました。

確認したい点
修正内容を誤って認識したかもしれません。
もしかしたら、「changePluginNameSpace」と「changePluginClassName 」を1つの関数でまとめれないかという意味だったでしょうか?

@IwasakiRyuichi
Copy link
Contributor Author

@ryuring

BcUtillに定義していたヘルパーのnamespaceとclassの変更処理を、ThemeServiceの方に移動しました。
ローカルでも正常に動作を確認できています。

Copy link
Collaborator

@ryuring ryuring left a comment

Choose a reason for hiding this comment

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

@IwasakiRyuichi レビューいれました!

$pluginPath = BcUtil::getPluginPath($newTheme);
if (!$pluginPath) return false;
if(file_exists($pluginPath . 'src/view/helper' . DS . 'Helper.php')) {
$helperClassPath = $pluginPath . 'stc/view/helper' . DS . $newTheme . 'Helper.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

src が stc になってます。
また、View と Helperはキャメルケースにしないと、linux環境では動作しないです。
(macは、大文字小文字を判別しないので動作します)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stcの箇所をsrcに修正しました。
ViewとHelperの修正ですが、view⇨View、helper⇨Helperのように修正しましたが、あってますでしょうか?・・

{
$pluginPath = BcUtil::getPluginPath($newTheme);
if (!$pluginPath) return false;
$oldTypePath = $pluginPath . 'src/View/Helper' . DS . 'Helper.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@IwasakiRyuichi $oldTypePath が存在することはないのでこちらは削除で大丈夫です。

$pluginPath = BcUtil::getPluginPath($newTheme);
if (!$pluginPath) return false;
$oldTypePath = $pluginPath . 'src/View/Helper' . DS . 'Helper.php';
$oldPath = $pluginPath . 'src/View/Helper' . DS . $oldTheme . 'Helper.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@IwasakiRyuichi /DS が混在しています。全て DS で統一しなければ、DS を使っている意味がないです。統一しましょう。

'src' . DS . 'View' . DS . 'Helper'

{
$pluginPath = BcUtil::getPluginPath($newTheme);
if (!$pluginPath) return false;
if(file_exists($pluginPath . 'src/View/Helper' . DS . 'Helper.php')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@IwasakiRyuichi この行の判定は不要です。

Copy link
Contributor Author

@IwasakiRyuichi IwasakiRyuichi Jan 8, 2025

Choose a reason for hiding this comment

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

@ryuring

ファイルパスの見直し、$oldTypePathの削除、373行目の判定削除の修正行いました。

今までDSの概念を分からずにいましたが、DSのおかげで異なるOS間でもファイルパスを適切にできることを知ることができました・・・ありがとうございます。

@ryuring ryuring merged commit 4ccca5c into baserproject:5.1.x Jan 8, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed レビュー済
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants