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

Type cast Ids from String to Integer #14057

Closed
wants to merge 1 commit into from

Conversation

yogeshsuhagiya
Copy link
Member

The methods getGroupId and getTaxClassId should set and return the Integer value of Id but they are returning String values.
And the method getSharedWebsiteIds should return integer type array of website Ids but it's returning the string type array of website Ids.
So before setting the value of Ids, I've typecast the value of Id from String to Integer.

Description

Fixed Issues (if relevant)

  1. Getting group id from customer session returns wrong data type. #14055: Getting group id from customer session returns wrong data type.

Manual testing scenarios

N/A

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@@ -886,7 +886,7 @@ public function getGroupId()
ScopeInterface::SCOPE_STORE,
$storeId
);
$this->setData('group_id', $groupId);
$this->setData('group_id', (int)$groupId);
}
return $this->getData('group_id');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why haven't you changed added casting from string to int here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not getting you, will you please elaborate?

Copy link

Choose a reason for hiding this comment

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

@yogeshks he is talking about the return. The return should be type casted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @ferrazzuk
Thanks for the correction, I'll update my commit accordingly.

Copy link

Choose a reason for hiding this comment

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

@yogeshks can you move the type casting to the return instead?

@ihor-sviziev ihor-sviziev self-assigned this Mar 12, 2018
@lbajsarowicz
Copy link
Contributor

I don't like the idea of change ID type to int - I still believe that other DBMS drivers (abstraction) are going to be developed for Magento sooner or later, then UUID can be required instead of Auto-Increment int value.

IMHO: We should keep getId() method for both Interface (you forgot) and Implementation class return type string.

PS: You have to keep in mind that change to interface can affect thousands of Magento 2 instances after update.

@bydrei
Copy link

bydrei commented Mar 12, 2018

@lbajsarowicz I disagree with you for these two reasons:

  1. If you have a look at the database (even thought the changes are not being made to the database) the type of group_id is of INT type. Same goes to the group tax id class and website id.
  2. Keeping it as a string doesn't keep the consistency of data types and also doesn't match the function comments and even the comments in the interface.

Its makes sense to keep data type's consistency in this case.

@yogeshsuhagiya
Copy link
Member Author

yogeshsuhagiya commented Mar 13, 2018

Hi @ferrazzuk,
Are my changes eligible for the merge with magento/magento2 branch? Will you please let me know.

@ihor-sviziev
Copy link
Contributor

Hi,
I don't mind to add those changes. Cc @orlangur
Any thoughts?

@bydrei
Copy link

bydrei commented Mar 13, 2018

@yogeshks I don't take those decision. I'm just a contributor like you

@orlangur orlangur self-assigned this Mar 13, 2018
@orlangur orlangur added this to the March 2018 milestone Mar 13, 2018
@orlangur
Copy link
Contributor

Closing PR as mentioned issue is not valid.

This issue is not valid, see #10519 (comment) for more details.

Thanks for collaboration @ferrazzuk and @yogeshks!

@orlangur orlangur closed this Mar 13, 2018
@ishakhsuvarov ishakhsuvarov deleted the Yogeshks-patch-1 branch March 14, 2018 15:08
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.

6 participants