Skip to content

Comments

Maker renew#1

Closed
lammn wants to merge 39 commits intomasterfrom
maker-renew
Closed

Maker renew#1
lammn wants to merge 39 commits intomasterfrom
maker-renew

Conversation

@lammn
Copy link

@lammn lammn commented Oct 26, 2016

For review

Copy link
Author

@lammn lammn left a comment

Choose a reason for hiding this comment

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

All review

.travis.yml Outdated
# re install plugin
- php app/console plugin:develop install --code=${PLUGIN_CODE}
# re enable plugin
- php app/console plugin:develop enable --code=${PLUGIN_CODE} No newline at end of file
Copy link
Author

Choose a reason for hiding this comment

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

Add new line of end file

if (!$TargetMaker) {
throw new NotFoundHttpException();
// Check request
if (!'POST' === $request->getMethod()) {
Copy link
Author

Choose a reason for hiding this comment

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

don't use 'POST' === $request->getMethod()

Choose a reason for hiding this comment

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

don't have form to check submit
=> don't fix.

@@ -25,12 +30,12 @@ class Maker extends \Eccube\Entity\AbstractEntity
private $name;
Copy link
Author

Choose a reason for hiding this comment

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

add variable comment

Choose a reason for hiding this comment

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

Check again because it was there.
=> don't change.

@@ -45,16 +50,10 @@ class Maker extends \Eccube\Entity\AbstractEntity
private $update_date;
Copy link
Author

Choose a reason for hiding this comment

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

add variable comment

Choose a reason for hiding this comment

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

Check again because it was there.
=> don't change.

return $this->getMethod();
}

private $id;
Copy link
Author

Choose a reason for hiding this comment

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

variable comment

/**
* @param Schema $schema
*/
protected function createPlgMakerForOldVersion(Schema $schema)
Copy link
Author

Choose a reason for hiding this comment

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

Add function comment


{% block title %}商品管理{% endblock %}
{% block sub_title %}メーカー管理{% endblock %}
{% block stylesheet %}
Copy link
Author

Choose a reason for hiding this comment

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

use css file

Choose a reason for hiding this comment

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

don't need.


</div>
</div>
{% endblock %} No newline at end of file
Copy link
Author

Choose a reason for hiding this comment

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

end of line

appveyor.yml Outdated
- php app/console plugin:develop enable --code=%PLUGIN_CODE%

test_script:
- vendor\bin\phpunit.bat app/Plugin/%PLUGIN_CODE%/Tests No newline at end of file
Copy link
Author

Choose a reason for hiding this comment

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

end of line

public function setUp()
{
parent::setUp();
$this->deleteAllRows(array('plg_product_maker', 'plg_maker'));
Copy link
Author

Choose a reason for hiding this comment

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

use constant for table name

Choose a reason for hiding this comment

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

Don't use constant because it do not reuse => don't fix.

@lqdung-lockon lqdung-lockon force-pushed the maker-renew branch 2 times, most recently from 940cd60 to 581097b Compare October 27, 2016 09:08
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66f25d2 on maker-renew into * on master*.

4 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66f25d2 on maker-renew into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66f25d2 on maker-renew into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66f25d2 on maker-renew into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66f25d2 on maker-renew into * on master*.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Changes Unknown when pulling 7f3c7df on maker-renew into * on master*.

@coveralls
Copy link

coveralls commented Dec 2, 2016

Coverage Status

Changes Unknown when pulling dbcc30f on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e862a0a on maker-renew into ** on master**.

2 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e862a0a on maker-renew into ** on master**.

@coveralls
Copy link

coveralls commented Dec 2, 2016

Coverage Status

Changes Unknown when pulling e862a0a on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

4 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

@coveralls
Copy link

coveralls commented Dec 2, 2016

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

2 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.

@coveralls
Copy link

coveralls commented Dec 2, 2016

Coverage Status

Changes Unknown when pulling 0cf014d on maker-renew into ** on master**.


// 一覧・登録・修正
$app->match('/' . $app["config"]["admin_route"] . '/product/maker/{id}', '\\Plugin\\Maker\\Controller\\MakerController::index')
$app->match('/'.$app['config']['admin_route'].'/plugin/maker/{id}', '\\Plugin\\Maker\\Controller\\MakerController::index')
Copy link

Choose a reason for hiding this comment

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

Don't add routing directory via $app. Use $admin->match instead of $app->match for force SSL support.
($appを使って直接ルーティングを追加するのではなく、強制SSLの対応のために$adminを使ってください。)

// 管理画面定義
$admin = $app['controllers_factory'];
// 強制SSL
if ($app['config']['force_ssl'] == Constant::ENABLED) {
    $admin->requireHttps();
}
// 一覧・登録・修正
$admin->match('/plugin/maker/{id}', '\\Plugin\\Maker\\Controller\\MakerController::index')
    ->value('id', null)->assert('id', '\d+|')
    ->bind('admin_plugin_maker_index');

...

$app->mount('/'.trim($app['config']['admin_route'], '/').'/', $admin);

See https://github.com/k-yamamura/ec-cube.github.io/blob/feature-plugin-bestpractices/plugin/controller.md#%E3%82%B3%E3%83%B3%E3%83%88%E3%83%AD%E3%83%BC%E3%83%A9%E3%83%BC%E3%81%AE%E3%83%AB%E3%83%BC%E3%83%86%E3%82%A3%E3%83%B3%E3%82%B0%E5%AE%9A%E7%BE%A9


// 上
$app->match('/' . $app["config"]["admin_route"] . '/product/maker/{id}/up', '\\Plugin\\Maker\\Controller\\MakerController::up')
$app->delete('/'.$app['config']['admin_route'].'/plugin/maker/{id}/delete', '\\Plugin\\Maker\\Controller\\MakerController::delete')
Copy link

Choose a reason for hiding this comment

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

$admin->delete

$app->match('/' . $app["config"]["admin_route"] . '/product/maker/{id}/down', '\\Plugin\\Maker\\Controller\\MakerController::down')
->value('id', null)->assert('id', '\d+|')
->bind('admin_maker_down');
$app->post('/'.$app['config']['admin_route'].'/plugin/maker/rank/move', '\\Plugin\\Maker\\Controller\\MakerController::moveRank')
Copy link

Choose a reason for hiding this comment

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

$admin->post

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1836f39 on maker-renew into ** on master**.

3 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1836f39 on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1836f39 on maker-renew into ** on master**.

@coveralls
Copy link

coveralls commented Dec 5, 2016

Coverage Status

Changes Unknown when pulling 1836f39 on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d759e20 on maker-renew into ** on master**.

3 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d759e20 on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d759e20 on maker-renew into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d759e20 on maker-renew into ** on master**.

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.

5 participants