Skip to content

Conversation

@beerbohmdo
Copy link
Contributor

@beerbohmdo beerbohmdo commented Nov 14, 2025

Before negative numbers was interpreted as possitive numbers.

Description

Manual testing steps

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli
Copy link
Member

Hi there,

This seems like it's probably a fine solution (though I haven't tested it yet) - since this is a bug with CMS 5.4, can you please retarget this PR to the 5.4 branch?

You'll probably also need to reset your commits after changing the target branch.

@beerbohmdo beerbohmdo changed the base branch from 6 to 5.4 November 18, 2025 07:00
@beerbohmdo beerbohmdo force-pushed the fix/parse_negative_numbers branch 2 times, most recently from 5fa8ac8 to e410f68 Compare November 18, 2025 07:10
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

This solution is fine for the standard case, but breaks down in an edge case where a class name (which can also be an injector name so usual class name rules may not apply) includes a hyphen.

For example if I add this YAML config:

SilverStripe\Core\Injector\Injector:
  Int-new:
    class: SilverStripe\ORM\FieldType\DBInt

and then I create this data object:

class MyDataObject extends DataObject
{
    private static $db = [
        'MyTest' =>  'Int-new(15)',
    ];
}

The default for that int field is -15 but it should be 15.

Before negative numbers was interpreted as possitive numbers.
@beerbohmdo beerbohmdo force-pushed the fix/parse_negative_numbers branch from e410f68 to b0a2a40 Compare November 21, 2025 07:31
@beerbohmdo
Copy link
Contributor Author

beerbohmdo commented Nov 21, 2025

I changed my method for checking for the minus, this should be more relyable.

BUT I don't think your edge case is realy one. Because:

Your example currently does not work also. It parses to: ["Int", [15]]. The new part is just ignored and for other names like "Foo-bar" it throws an Invalid T_STRING "bar"-error.

Btw. if you swap the - to a dot (which should work) Foo.bar is handled correctly, but Int.new gets Int. :/

So we need a better way to handle the class/identifier part. But I am not sure if this is part of this issue. Give me the ok and I will look into that.

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