Skip to content

Conversation

@vipin1190
Copy link

Closes issue #39 "Account numbers with leading zeros are stripped when cast to int"

@code2prog
Copy link
Contributor

@vipin1190 I’ll take a look at it when I have some free time. Thank you for your contribution

@ash-g-1337
Copy link
Contributor

ash-g-1337 commented Dec 10, 2024

Quick Question: How will the change behave when you pass in an INT as most of the people are likely passing INT. May want to create unit test to rule out that scenario; if it throws type exception or something, either it should be handled or we should note a breaking change so people can make the right level of change in their code before they pull into their existing codebase.

In most cases php handles the type casting, but it sometimes behaves differently on different platforms.

I will try to pull and test it when I have the time.

@code2prog
Copy link
Contributor

code2prog commented Dec 10, 2024

@ash-g-1337: For now, perhaps the best idea would be to allow both types (int and string) in the setAccountNumber method and, depending on the type, cast it to a string with additional operations such as filling in missing leading zeros and validating whether the account number meets the requirements.

    if (is_int($accountNumber)) {
        $accountNumber = str_pad((string)$accountNumber, 9, '0', STR_PAD_LEFT);
    }
    if (!is_string($accountNumber) || !preg_match('/^\d{9}$/', $accountNumber)) {
        throw new InvalidArgumentException('Invalid FedEx account number format. Must be a 9-digit number.');
    }

@ash-g-1337
Copy link
Contributor

@vipin1190 @code2prog : Having both makes sense to allow backwards compatibility, just do the casting based on what is being passed.
as a warning, we can document that when passing INT account validation would be put on server side.. eg. someone makes a typo and puts in 5 digit INT (eg. 000012345 and 12345 as INT would be 12345).. str_pad will add the 0s to make it 9 and look like a valid account number even if it is not. At that point, FedEx API will likely through error assuming the account number is not valid or the credentials don't have access to the given account.

@ash-g-1337
Copy link
Contributor

I was working on another project and discovered something interesting.. as of php 8.1 INT starting with leading 0 (zero) is treated as Octal number.. when you cast, it converts the value from octal.. eg. when converting 01000 it returns 512 when you cast as string.

@ash-g-1337
Copy link
Contributor

@vipin1190 @code2prog
Was doing some local testing and looks like changes you made wont have much impact with existing implementation (though we should mention it as part of release).

Only scenario that I ran into some code breaking was where someone had had coded the account number <-not the best practice for production code.

Adding additional check for int and length as mentioned by @code2prog should be enough.

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.

3 participants