Skip to content

[WIP] Improved conversion for aliased classnames #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 16, 2015
Merged

[WIP] Improved conversion for aliased classnames #351

merged 3 commits into from
Feb 16, 2015

Conversation

@phansys
Copy link
Contributor

@phansys phansys commented Feb 1, 2015

Improved conversion for aliased classnames

Before:
Doctrine\Tests\Common\Persistence\Mapping:ChildEntity:Foo resolves (silently) to Doctrine\Tests\Common\Persistence\Mapping\ChildEntity

After:
Doctrine\Tests\Common\Persistence\Mapping:ChildEntity:Foo resolves to Doctrine\Tests\Common\Persistence\Mapping\ChildEntity:Foo in order to get validation in lower layers.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR none

TODO:

  • Add tests for AbstractClassMetadataFactory::getMetadataFor()
  • Add tests for MappingDriver::isTransient()
  • Add tests for AbstractManagerRegistry::getManagerForClass()

@phansys
Added validation where allowed QCNs with ":" NS separator.
f6f0ae5 
Before:
```MyEntityNamespace:Foo:Bar``` resolves (silently) to ```MyEntityNamespace:Foo```

After:
```MyEntityNamespace:Foo:Bar``` throws ```\InvalidArgumentException```

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | none
@doctrinebot
Copy link

doctrinebot commented Feb 1, 2015

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-270

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

MyEntityNamespace:Foo:Bar resolves (silently) to MyEntityNamespace:Foo

MyEntityNamespace:Foo is an invalid class name (will fail to autoload) anyway, no?

Ocramius
Ocramius reviewed Feb 1, 2015
View reviewed changes
lib/Doctrine/Common/Persistence/AbstractManagerRegistry.php Outdated
Copy link
Member

@Ocramius Ocramius Feb 1, 2015

Choose a reason for hiding this comment

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

Note that explode() has a third $limit argument (see http://3v4l.org/as2I7), maybe that's a better solution here...

Copy link
Contributor Author

@phansys phansys Feb 1, 2015

Choose a reason for hiding this comment

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

Yes @Ocramius, thank you for the tip.
I updated the PR with this change.

@phansys
Copy link
Contributor Author

phansys commented Feb 1, 2015

Yes @Ocramius, of course MyEntityNamespace:Foo is an invalid CN. Sorry for the misunderstood, I did mean that these parts of the given alias were catched while the convertion to the real CN (MyEntityNamespace\Foo in my example). BTW, I updated the examples in the PR message in order to reflect the current status.

Now I need some hint in order to finish the 2nd and 3rd item.
Thank you.

@phansys phansys changed the title Added validation where allowed QCNs with ":" NS separator [WIP] Improved conversion for aliased classnames Feb 1, 2015
@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

Tests for isTransient and for the AbstractManagerRegistry should also be provided

@Ocramius Ocramius self-assigned this Feb 1, 2015
@phansys
Copy link
Contributor Author

phansys commented Feb 1, 2015

Sure, this is the reason of the updated title with "[WIP]" prepended; but I tried to add a test for isTransient() without success because the driver isn't validating the classname.

Test:

public function testInvalidClassIsTransient()
{
    $this->setExpectedException(
        'Doctrine\Common\Persistence\Mapping\MappingException',
        'Class \'Doctrine\Tests\Common\Persistence\Mapping\ChildEntity:Foo\' does not exist'
    );

    $this->cmf->isTransient('prefix:ChildEntity:Foo');
}

results in:

There was 1 failure:

1) Doctrine\Tests\Common\Persistence\Mapping\ClassMetadataFactoryTest::testInvalidClassIsTransient
Failed asserting that exception of type "Doctrine\Common\Persistence\Mapping\MappingException" is thrown.

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

@phansys should just report true, tbh

@phansys
Copy link
Contributor Author

phansys commented Feb 1, 2015

Perfect @Ocramius, I will add a test for this case with that assert.
What about AbstractManagerRegistry? Do you have in mind some existent test in which this one could be appended?

@Ocramius
Copy link
Member

Ocramius commented Feb 2, 2015

@phansys thanks for providing tests. I'll review and merge manually after fixing nitpickery myself.

@phansys
Copy link
Contributor Author

phansys commented Feb 2, 2015

You're welcome @Ocramius!

Ocramius added a commit that referenced this pull request Feb 16, 2015
@Ocramius
Merge pull request #351 from phansys/repo_check_classname_alias
f1004b8 
[WIP] Improved conversion for aliased classnames
@Ocramius Ocramius merged commit f1004b8 into doctrine:master Feb 16, 2015
@Ocramius
Copy link
Member

Ocramius commented Feb 16, 2015

Merged, thanks!

@doctrinebot doctrinebot mentioned this pull request Dec 6, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Ocramius Ocramius

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@phansys @doctrinebot @Ocramius