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

Feature/php 81 #178

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Feature/php 81 #178

wants to merge 12 commits into from

Conversation

w911
Copy link

@w911 w911 commented Apr 3, 2023

No description provided.

@w911 w911 requested review from samdark and cgsmith April 3, 2023 15:30
Comment on lines +21 to +22
final const PAGE_NUMBER = 0;
final const PAGE_SIZE = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
final const PAGE_NUMBER = 0;
final const PAGE_SIZE = 10;
private const PAGE_NUMBER = 0;
private const PAGE_SIZE = 10;

Copy link
Collaborator

Choose a reason for hiding this comment

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

But please ensure that it's not used externally.

Comment on lines +24 to +25
final CONST PAGE_SIZE_MIN = 0;
final const PAGE_SIZE_MAX = 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
final CONST PAGE_SIZE_MIN = 0;
final const PAGE_SIZE_MAX = 1000;
private CONST PAGE_SIZE_MIN = 0;
private const PAGE_SIZE_MAX = 1000;

@@ -148,7 +148,7 @@ public function actionIndex()
* @throws \yii\base\InvalidConfigException
* @throws ForbiddenHttpException
*/
public function actionCreate()
public function actionCreate(): array|\api\modules\v1\models\customer\CustomerEx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return type is wrong.

@@ -238,7 +238,7 @@ public function actionCreate()
* @return array|\api\modules\v1\models\customer\CustomerEx
* @throws ForbiddenHttpException
*/
public function actionView($id)
public function actionView($id): array|\api\modules\v1\models\customer\CustomerEx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return type is wrong.

@@ -325,7 +325,7 @@ public function actionView($id)
* @throws \yii\base\InvalidConfigException
* @throws ForbiddenHttpException
*/
public function actionUpdate($id)
public function actionUpdate($id): array|\api\modules\v1\models\customer\CustomerEx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return type is wrong.

@@ -38,8 +38,8 @@ public function behaviors(): array

public function init(): void
{
$this->on(self::EVENT_AFTER_UPDATE, [$this, 'createJobIfNeeded']);
$this->on(self::EVENT_AFTER_INSERT, [$this, 'createJobIfNeeded']);
$this->on(self::EVENT_AFTER_UPDATE, $this->createJobIfNeeded(...));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks wrong.

@@ -53,7 +53,7 @@ public function scenarios(): array

public function init(): void
{
$this->on(self::EVENT_BEFORE_INSERT, [$this, 'orderHistoryPopulate']);
$this->on(self::EVENT_BEFORE_INSERT, $this->orderHistoryPopulate(...));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks wrong. Let's search for (...) everywhere.

@@ -448,6 +449,8 @@ public function actionClone($id)
*/
public function actionDelete($id)
{
$model = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for it.

@@ -448,6 +449,8 @@ public function actionClone($id)
*/
public function actionDelete($id)
{
$model = null;
$transaction = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for it. It is initialized properly when used. Similar in

@@ -772,7 +775,7 @@ public function actionImport()

$model = new OrderImport();

if (count($customers) === 1) {
if ((is_countable($customers) ? count($customers) : 0) === 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

$customers can't be non-array, no need for this check.

@samdark
Copy link
Collaborator

samdark commented Apr 3, 2023

Seems @cebe did the same stuff in his own PR that is now merged into master. @w911 please merge master into this PR and we'll see if there's anything useful left.

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.

2 participants