Correctly return int values where required.

[ghost]

Related Pull Requests

https://github.com/magento/partners-magento2ee/pull/159
https://github.com/magento/partners-magento2b2b/pull/61

Description (*)

The methods getCustomerId and getCustomerGroupId incorrectly return strings due to the usage of getData. This PR casts the values to ints, to match the return type hints.

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 are green)

[m2-assistant]

Hi @paul-gene. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[ihor-sviziev]

@paul-gene,
Your changes looks good, however there are some test failures, looks like your changes broke something.
Could you fix found issues?

Pull Request state was updated. Re-review required.

[ghost]

@ihor-sviziev are the tests failing due to my actual code change?

[ihor-sviziev]

@paul-gene seems like yes, but let’s restart failing tests, maybe it was some issue with testing infra
@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests

[ghost]

@ihor-sviziev If my code change is the cause for failing tests, then the problem is bigger than I thought, as it means tests were written to be broken from the start. fml.

Can the tests be re-run without a commit to kick them into action?

[ihor-sviziev]

@paul-gene TBH it's not seems the change chat could break functionality, but it might in not obvious way. All functional and integration tests are running all the time for all PRs, and I didn't saw that these tests were failing somewhere else.

[ghost]

@ihor-sviziev I can see a few PRs around the same time as mine that are failing the same tests.

[ghost]

@ihor-sviziev there is definitely something fishy going on.

https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/26930/f673188cf2c549b9e5484797c046e05f/Integration/allure-report-ce/index.html#suites/f7191fe56ccb02e8d0dc974559e41db3/8dcae0b2bb1cc1d5/

No way my code change broke this test:

"Redirection URL does not match expectations
Failed asserting that 'http://localhost/index.php/customer/address/index/' contains "customer/address/edit"."

And https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/26930/f673188cf2c549b9e5484797c046e05f/Integration/allure-report-ce/index.html#suites/f7191fe56ccb02e8d0dc974559e41db3/f94ed93e74f071d7/

The "expected" result looks to be empty in the test results. I don't see how my change could have broken that?

[magento-engcom-team]

Hi @VladimirZaets, thank you for the review.
ENGCOM-6947 has been created to process this Pull Request
✳️ @VladimirZaets, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[magento-engcom-team]

@paul-gene thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[ghost]

@VladimirZaets what's the next step to get this merged?

[ghost]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests

[ihor-sviziev]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests

@paul-gene this command available only for maintainers

[ghost]

Thanks @ihor-sviziev

Are you able to explain this though? -#26930 (comment)

From what I can tell (allure is terrible to read), there is no possible way my code change could be breaking some of those tests.

[ihor-sviziev]

@magento give me test instance

[magento-engcom-team]

Hi @ihor-sviziev. Thank you for your request. I'm working on Magento instance for you

[slavvka]

@paul-gene Unfortunately we have an error in our performance sanity check. For some reason opening the customer edit page fails:

1587742244099,240,Edit Customer (Admin Customer Management),200,OK,One Thread Scenarios Pool 5-1,text,false,Response was null,112515,0,1,1,http://localhost/builds/backend/customer/index/edit/id/1/,236,0,0

Could you please check what is the reason of that. You need to run jMeter scenario against your code (sanity (3).zip).

More information you can find here https://github.com/magento/magento2ce/tree/2.4-develop/setup/performance-toolkit. There's a mistake in there, you need to take 3rd version

Pull Request state was updated. Re-review required.

[ihor-sviziev]

Hi @paul-gene,
I don't see any changes, so not sure we do have something to review. Maybe you missed to push your changes?

[ghost]

@ihor-sviziev honestly, I don't understand any of @slavvka 's comment.

Everything was approved and all tests passed, so how all of a sudden did something break?

[ihor-sviziev]

@paul-gene your changes looks good, but we during running performance tests we found performance degradation in one specific case (that @slavvka described above). You could reproduce it by running attached jMeter profile from and compare changes from your PR with 2.4-develop branch.

This is one of the last steps before merging PR, that's why it happened.

Unfortunately we should strong motivation for accepting PRs that introduce performance degradation, while this PR don't have such, so it seems like PR should be adjusted.

Hope you got my point. If you have any questions

BTW I found that in comment above we have incorrect link, here is correct one https://github.com/magento/magento2/tree/2.4-develop/setup/performance-toolkit

[ghost]

@ihor-sviziev I'll be honest, a lot of work was done after my initial commits, and I've never heard of, or used, jMeter.

I'm not sure I believe that casting variables to integers would degrade performance enough for it to make any conceivable difference, if at all :/

[slavvka]

@engcom-Echo could you please help @paul-gene find out what's wrong with the PAT Sanity scenario?

[ihor-sviziev]

@magento run all tests

[orlangur]

Please provide test scenarios in which returning int is required (i.e. broken functionality).

int type hint covers both integers and numeric strings in fact.

See #14057 (comment) for more details.

[ihor-sviziev]

Hi @orlangur,
Will copy my response here (from #10519 (comment)):

Issue appears when you're using strict types declare(strict_types=1) in your file. Then you expect that if value should be int - you can use it for your method that has declaration like this

<?php
declare(strict_types=1);

function myFunction(int $storeId): void
{
}

$storeId = '2'; // let's assume we executed $store->getId(); that has defined return type int
myFunction($storeId);

output of php 1.php:

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to myFunction() must be of the type integer, string given, called in /home/ihor/1.php on line 7 and defined in /home/ihor/1.php:3
Stack trace:
#0 /home/ihor/1.php(7): myFunction('2')
#1 {main}
  thrown in /home/ihor/1.php on line 3

In this case you'll get type error. Why - because string !== int.

[orlangur]

@ihor-sviziev this code is incorrect, we cannot rely on types derived from files without strict types declaration.

@paul-gene as per discussion in maintainers chat I'm closing this PR. We must use strict types in all new code, however, we have too many numeric strings in legacy code and any such change may cause bugs.

[m2-assistant]

Hi @paul-gene, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.