Problem/Motivation

Number::alphadecimalToInt() triggers a deprecation when called with a malformed string:

   DEPRECATED  Invalid characters passed for attempted conversion, these have been ignored in core/lib/Drupal/Component/Utility/Number.php on line 98.

Steps to reproduce

\Drupal\Component\Utility\Number::alphadecimalToInt("à");

Proposed resolution

The code should probably make sure substr($string, 1) returns a valid value before feeding it to base_convert().

Issue fork drupal-3442810

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

pradhumanjain2311 made their first commit to this issue’s fork.

Sandeep Sanwale made their first commit to this issue’s fork.

Sandeep Sanwale opened merge request !7843

sandeep sanwale’s picture

sandeep sanwale commented

Here i have added the check which validates that if $string contains malformed string then it throws an exception .

Gaurav Gupta made their first commit to this issue’s fork.

sandeep sanwale’s picture

sandeep sanwale commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

vidorado made their first commit to this issue’s fork.

vidorado’s picture

vidorado commented

Many tests were failing because they were passing NULL or an empty string to Number::alphadecimalToInt(). So I believe we should continue accepting these two special "degenerate" values as input. Two tests have been added: one to ensure this behavior and other to verify that an exception is thrown for any other non-alphanumeric characters.

I'm not sure how to proceed if we want to provide a deprecation message. Perhaps something like this?

@trigger_error('Passing non-alphanumeric characters is deprecated in Number::alphadecimalToInt() and will be removed in Drupal 12.', E_USER_DEPRECATED);

vidorado’s picture

vidorado commented
Status: Needs work » Needs review
Issue tags: -Needs tests
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

So believe we should actually do a deprecation here. #10 close but there's a specific pattern that has to be followed, would check core for examples. But deprecated in 11.2 and required in 12.

vidorado’s picture

vidorado commented
Status: Needs work » Needs review

I've created a change record and triggered a deprecation error, as it must be done according to https://www.drupal.org/node/2856615

Additionally, the follow-up issue #3494476: Remove NumberTest::testAlphadecimalToIntReturnsZeroWithNullAndEmptyString() test has been created in order to remove the BC test.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Would be deprecated in 11.2

Can't add deprecation for versions with releases out already.

vidorado’s picture

vidorado commented
Status: Needs work » Needs review

Oops, sorry! :) I wasn’t sure which version to include in the message since I don’t really know when this MR will be merged. I initially put 11.0.0 and later forgot to revisit it.

So, should we always deprecate for the next minor version as an estimate?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Believe feedback has been addressed.

Test covers the new exception, deprecation message for both '' and null.
CR reads fine to me, added versions to it.

LGTM

  • catch committed c88bf78b on 11.x 
    Issue #3442810 by vidorado, gaurav gupta, sandeep sanwale, smustgrave,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !7843

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.