Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

On SMv2 only canonicalized aliases make sense #69

Merged
merged 1 commit into from
May 3, 2016

Conversation

Slamdunk
Copy link
Contributor

@Slamdunk Slamdunk commented May 2, 2016

All aliases are checked canonicalized.
Non-canonicalized only aliases raise a missin-alias error.
Other non-canonicalized are useless.

$aliases = $r->getValue($this->manager);

foreach ($aliases as $name => $alias) {
$this->manager->get($name . ' ');
Copy link
Contributor

@zluiten zluiten May 2, 2016

Choose a reason for hiding this comment

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

Quite expensive no? An assertion on lower casing should be sufficient I think. $this->assertSame(strtolower($name), $name);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the bug because I was trying to get the element with the camel-cased key.
As such, the test should reflect the use case and treat the code in the best black-box way possible.
Checking the case of keys would be a total white-box test with no insight of why we are testing that way.

@weierophinney weierophinney added this to the 2.8.3 milestone May 3, 2016
@weierophinney weierophinney self-assigned this May 3, 2016
@weierophinney weierophinney merged commit 48514c7 into zendframework:master May 3, 2016
weierophinney added a commit that referenced this pull request May 3, 2016
On SMv2 only canonicalized aliases make sense
weierophinney added a commit that referenced this pull request May 3, 2016
weierophinney added a commit that referenced this pull request May 3, 2016
weierophinney added a commit that referenced this pull request May 3, 2016
@Slamdunk Slamdunk deleted the test/aliases-v2 branch May 12, 2016 07:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants