Ticket #979 (closed 新規開発: 修正済)

Opened 11 years ago

Last modified 11 years ago

[フロント]会員登録

Reported by: kotani Owned by: kimoto
Priority: Milestone: EC-CUBE2.11.0 リファクタリングProject
Component: フロント Version: 2.11.0 β
Keywords: Cc:
修正済み:

Description

▼リファクタリング対象ファイル
/entry/LC_Page_Entry.php
/entry/LC_Page_Entry_Complete.php
/entry/LC_Page_Entry_EmailMobile.php
/entry/LC_Page_Entry_Kiyaku.php
/regist/LC_Page_Regist.php
/regist/LC_Page_Regist_Complete.php


リファクタリングガイドライン
http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン


Attachments

#979_lfMakeSqlVal_11020401.2.php Download (1.5 KB) - added by kajiwara 11 years ago.

Change History

comment:1 Changed 11 years ago by kimoto

  • Owner changed from somebody to kimoto
  • Status changed from new to assigned

comment:2 follow-up: ↓ 3 Changed 11 years ago by kajiwara

kimoto様

株式会社ロックオンの梶原でございます。

会員登録という巨大な部分にアサインいただき、誠に恐縮です。
何卒よろしくお願い致します!

まず現時点で、一度動作確認をさせていただきました。

■仮会員登録 1.管理画面>システム設定>パラメータ設定 で、「CUSTOMER_CONFIRM_MAIL」を「true」に変更。(仮会員ONにする)
2.会員登録
3.本下院登録案内メール本文にあるURLへアクセスするとエラー

仮会員登録の3のところでエラーとなります。
申し訳ございませんが、ご確認いただけると幸いです。

また、別途ソースの確認を行い、フィードバックさせていただきます。

リファクタリングを開始したばかりで、色々分からないことがあると思いますが、随時フィードバックさせていただきますので、何卒よろしくお願い致します!!

comment:3 in reply to: ↑ 2 Changed 11 years ago by kimoto

株式会社ロックオンの梶原さま

■仮会員登録 1.管理画面>システム設定>パラメータ設定 で、「CUSTOMER_CONFIRM_MAIL」を「true」に変更。(仮会員ONにする)
2.会員登録
3.本下院登録案内メール本文にあるURLへアクセスするとエラー

仮会員登録の3のところでエラーとなります。
申し訳ございませんが、ご確認いただけると幸いです。

もうしわけありません、確認漏れでした。 修正しましたのでご確認ください。

量があって時間がかかっていますが、なんとかしたいと思いますので今後ともよろしくお願いします

Changed 11 years ago by kajiwara

comment:4 Changed 11 years ago by kajiwara

kimoto様
株式会社ロックオンの梶原でございます。

今回、リファクタリングガイドラインが結構多岐にわたっており、そんな中で作業いただいており、本当にありがとうございます。
というところで、少し時間がかかってしまいましたが、ソースの確認をさせていただきました。

以下の点がリファクタリングガイドラインから外れておりますので、修正いただけるとありがたいです。

(1)http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン#端種別の判別

  • Net_UserAgent_Mobile::isMobile() や MOBILE_SITE 定数など存在するが, SC_Display::detectDevice() に統一する

(2)http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン#データベースアクセス

  • 特に理由の無い場合はSC_Query::getSingletonInstance() を使用してインスタンスを取得すること

(3)http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン#ビジネスロジック

  • 宣言を除き, 引数や返り値が無く, すべて内部のメンバ変数で処理するような関数は極力作成しない

こちらは分かりにくいと思いますので、lfMakeSqlVal の修正方法を例にとり、説明させていただきます。
修正したファイルは本チケットに添付させていただきました。

<?php

/**
 * lfMakeSqlVal XXX 関数名は無くても良い
 *
 * 会員登録に必要なsqlを作成する
 *
 * XXX
 * 実際にSQL文を生成するけではないので「会員登録に必要なSQLパラメータを生成する」という概要の方が良いと思われる
 *
 * @access public XXX 外部からのアクセスは想定しないと思われるので protected では?
 * @return void XXX 連想配列を返しているので array SQLパラメータの配列 等とすべき
 */
function lfMakeSqlVal() {
    /*
     * XXX
     * 変数名 $arrRet は, どのようなデータか判別できないので不適切
     * 入出力が解らなくなるので, getHashArray() の結果と getDbArray() の結果を, 関数の引数で受けとるべき
     */
    $arrRet     = $this->objFormParam->getHashArray();
    $sqlval     = $this->objFormParam->getDbArray();

    // 登録データの作成 XXX 生年月日を作成している
    $sqlval['birth']  = SC_Utils_Ex::sfGetTimestamp($arrRet['year'], $arrRet['month'], $arrRet['day']);

    // 仮会員 1 本会員 2
    $sqlval["status"] = (CUSTOMER_CONFIRM_MAIL == true) ? "1" : "2";

    /*
      secret_keyは、テーブルで重複許可されていない場合があるので、
      本会員登録では利用されないがセットしておく。
    */
    $sqlval["secret_key"] = SC_Helper_Customer_Ex::sfGetUniqSecretKey();        // 会員登録キー

    $CONF = SC_Helper_DB_Ex::sfGetBasisData();
    $sqlval["point"] = $CONF["welcome_point"]; // 入会時ポイント

    // XXX SC_Display::detectDevice() == DEVICE_TYPE_MOBILE を使用する
    if ($this->isMobile === true) {
        // 携帯メールアドレス
        $sqlval['email_mobile']     = $sqlval['email'];
        //PHONE_IDを取り出す
        $sqlval['mobile_phone_id']  =  SC_MobileUserAgent::getId();
    }

    return $sqlval;
}

?>

主に、引数や返り値が無く、メンバ変数の値を直接変更している点においての違反となっておりまして、引数や返り値で制御ができる関数であることが望ましいという意図です。

お忙しいところ大変申し訳ございませんが、ご確認いただき、再度チャレンジいただけませんでしょうか。

会員登録分、量が多く、恐縮ですが、何卒よろしくお願い致します!!

comment:5 Changed 11 years ago by kajiwara

kimoto様

株式会社ロックオンの梶原でございます。

もう一点、以下の部分もご確認いただければありがたいと思います。
returnせずに関数内に遷移が入っているものがありましたのでご報告します。

以下、コメントを入れております。

<?php
/**
 * lfCheckReferer
 * XXX 関数の概要を記載する
 *
 * @access public XXX 外部からのアクセスは想定されていないと思われるので protected では?
 * @return void XXX 遷移の可否を boolean で返して, 遷移は action 関数で行う
 */
function lfCheckReferer(){
    /**
     * 規約ページからの遷移でなければエラー画面へ遷移する
     * XXX 関数の説明は, 関数コメントに記載する
     */
    if ($this->isMobile === FALSE // XXX SC_Display::detectDevice() を使用する
        && empty($_POST) // XXX 直接スーパーグローバル配列を使用しない
        && !preg_match('/kiyaku.php/', basename($_SERVER['HTTP_REFERER'])) // XXX 直接スーパーグローバル配列を使用しない
        ) {
        // XXX 遷移は action 関数で行うこと
        SC_Utils_Ex::sfDispSiteError(PAGE_ERROR, "", true);
    }
}

?>

こちらを直すと以下のようになるかと思いますので、是非ご参照ください。

<?php
/**
 * kiyaku.php からの遷移の妥当性をチェックする
 *
 * 以下の内容をチェックし, 妥当であれば true を返す.
 * 1. 規約ページからの遷移かどうか
 * 2. PC及びスマートフォンかどうか
 * 3. $post に何も含まれていないかどうか
 *
 * @access protected
 * @param array $post $_POST のデータ
 * @param string $referer $_SERVER['HTTP_REFERER'] のデータ
 * @return boolean kiyaku.php からの妥当な遷移であれば true
 */
function lfCheckReferer(&$post, $referer){

    if (SC_Display::detectDevice() != DEVICE_TYPE_MOBILE
        && empty($post)
        && !preg_match('/kiyaku.php/', basename($referer))) {
        return true;
    } else {
        return false;
    }
}

?>

それでは、どうぞよろしくお願い致します!

comment:6 Changed 11 years ago by kimoto

株式会社ロックオンの梶原さま

ご指摘ありがとうございます /entry/LC_Page_Entry.phpを修正してみました。

        // パラメータ管理クラス,パラメータ情報の初期化
        $this->objFormParam = new SC_FormParam();
        $this->lfInitParam();

は初期化と解釈してinit()内で行っておりますが、action内で定義すべきでしょうか?

comment:7 Changed 11 years ago by kajiwara

kimoto様
本当にありがとうございます!

そして、とても良いご指摘、誠にありがとうございます。

ご指摘ありがとうございます /entry/LC_Page_Entry.phpを修正してみました。

        // パラメータ管理クラス,パラメータ情報の初期化
        $this->objFormParam = new SC_FormParam();
        $this->lfInitParam();

は初期化と解釈してinit()内で行っておりますが、action内で定義すべきでしょうか? 

そうですね。
このコードは少し厄介なことに、ページごとで呼ぶタイミングが異なってる場合があるので、ここはaction内で行うということで統一したいと思います。

こちらのソースが参考になると思います。
http://svn.ec-cube.net/open_trac/changeset/20071

よろしくお願い致します。

comment:8 Changed 11 years ago by kimoto

  • Status changed from assigned to closed
  • Resolution set to 修正済

comment:9 follow-up: ↓ 12 Changed 11 years ago by nanasess

  • Status changed from closed to reopened
  • Resolution 修正済 deleted
  • branches/version-2_5-dev/data/class/pages/entry/LC_Page_Entry_EmailMobile.php

     
    121124 
    122125    /** 
    123      * lfRegister 
    124126     * 
     127     * 携帯メールアドレスが登録されていないユーザーに携帯アドレスを登録する 
     128     * 
     129     * 登録完了後にsessionのemail_mobileを更新する 
     130     * 
     131     * @param mixed $objFormParam 
    125132     * @param mixed $objCustomer 
    126      * @access public 
     133     * @access private 
    127134     * @return void 
    128135     */ 
    129     function lfRegister(&$objCustomer) { 
    130         $customer_id    = $objCustomer->getValue('customer_id'); 
    131         $email_mobile   = strtolower($this->objFormParam->getValue('email_mobile')); 
    132  
    133         $objQuery       = new SC_Query(); 
    134         $objQuery->update('dtb_customer', array('email_mobile' => $email_mobile), 'customer_id = ?', array($customer_id)); 
     136    function lfRegistEmailMobile($email_mobile, $customer_id) { 
     137        $objQuery = SC_Query::getSingletonInstance(); 
     138        $objQuery->update('dtb_customer', 
     139                          array('email_mobile' => $email_mobile), 
     140                          'customer_id = ?', array($customer_id)); 
    135141 
    136142        $objCustomer->setValue('email_mobile', $email_mobile); 

LC_Page_Entry_EmailMobile::lfRegistEmailMobile() で SC_Customer を参照しているため, エラーが発生するようです.

comment:10 follow-up: ↓ 11 Changed 11 years ago by nanasess

/entry/LC_Page_Entry_Kiyaku.php で, Net_UserAgent_Mobile::isMobile() が使用されています

comment:11 in reply to: ↑ 10 Changed 11 years ago by kimoto

nanasess への返信

/entry/LC_Page_Entry_Kiyaku.php で, Net_UserAgent_Mobile::isMobile() が使用されています

ありがとうございます 修正しました

comment:12 in reply to: ↑ 9 Changed 11 years ago by kimoto

  • Status changed from reopened to closed
  • Resolution set to 修正済

LC_Page_Entry_EmailMobile::lfRegistEmailMobile() で SC_Customer を参照しているため, エラーが発生するようです.

凡ミスですみません。修正しました

Note: See TracTickets for help on using tickets.