Class attribute name validity check #855

Closed
destructivecreator opened this Issue Mar 11, 2013 · 4 comments

2 participants

@destructivecreator

Just sharing something I have noticed recently. I think that the following check can be added to assertClassHasAttribute()and the rest similar to it.

Currently this (notice that 1 is not a valid name for class attribute $1:

 $this->assertClassHasAttribute('1', 'ClassWithAttributes');

Would result in the following:
Failed asserting that class "ClassWithAttributes" has attribute "1".

According to the PHP manual:
"Variable names follow the same rules as other labels in PHP. A valid variable name
starts with a letter or underscore, followed by any number of letters, numbers, or
underscores. As a regular expression,
it would be expressed thus: '[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*' "

I propose the following check inside the assertion method:

 if(!preg_match('/[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*/', $attribute)) {
            throw PHPUnit_Util_InvalidArgumentHelper::factory(1, 'a valid attribute name');
 }

If you consider it valuable I can do a pull and commit the changes.

@whatthejeff
Collaborator

I'll accept a PR against master for this. Don't forget to provide the appropriate tests as well. Thanks :)

@destructivecreator

As there are several places that require attribute validation, would you accept if I refactor the existing code and create new method inside Assert.php that encapsulates the validation and use it in all appropriate places, or just to add an if statement to the existing assertions ?

Thanks

    protected static function checkAttributeValidity($attributeName, $argumentPosition)
    {
        if (!is_string($attributeName)) {
            throw PHPUnit_Util_InvalidArgumentHelper::factory($argumentPosition, 'string');
        }


        if(!preg_match('/[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*/', $attributeName)) {
            throw PHPUnit_Util_InvalidArgumentHelper::factory($argumentPosition, 'a valid name of attribute');
        }
    }
@whatthejeff
Collaborator

Let's stay consistent and add if statements to the existing assertions. I don't see much value in a refactor at this time.

@destructivecreator destructivecreator added a commit to destructivecreator/phpunit that referenced this issue Mar 13, 2013
@destructivecreator destructivecreator Changes made for issue #855:
- Added additional validation of attribute name in assertions using class/object attributes
- Added new Test in Tests/Framework/AssertTest.php to cover attribute validation
8d44664
@whatthejeff
Collaborator

Closed by 8d44664.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment