Skip to content
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.

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

Appends cache namespace when it exists (for L2C regions) #6223

Merged
merged 2 commits into from
Jan 11, 2017

Conversation

lcobucci
Copy link
Member

We're overriding the namespace without even checking if it was previously set, what causes problems when people uses that feature 😉

@lcobucci lcobucci added the Bug label Jan 11, 2017
@lcobucci lcobucci self-assigned this Jan 11, 2017
@lcobucci lcobucci added this to the 2.6.0 milestone Jan 11, 2017
@lcobucci
Copy link
Member Author

@Ocramius should we also port it for 2.5.x?

$factory = new DefaultCacheFactory($this->regionsConfig, $cache);

$fooRegion = $factory->getRegion(
[
Copy link
Member

Choose a reason for hiding this comment

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

Minor alignment issues here


$namespace = $cacheAdapter->getNamespace();

if (!empty($namespace)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use '' === $namespace, unless getNamespace only allows string as return value

]
);
$barRegion = $factory->getRegion(
[
Copy link
Member

Choose a reason for hiding this comment

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

Alignment issues here

);

$this->assertSame('testing:foo', $fooRegion->getCache()->getNamespace());
$this->assertSame('testing:bar', $barRegion->getCache()->getNamespace());
Copy link
Member

Choose a reason for hiding this comment

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

Are prefixes for non-namespaced caches checked in some existing tests??

Copy link
Member Author

@lcobucci lcobucci Jan 11, 2017

Choose a reason for hiding this comment

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

@Ocramius
Copy link
Member

Ocramius commented Jan 11, 2017

@Ocramius should we also port it for 2.5.x?

@lcobucci no, 2.6, as this actually changes cache prefixes, and may lead to deployment issues.

We're overriding the namespace without even checking if it was previously
set, what causes problems when people uses that feature 😉
@lcobucci lcobucci dismissed Ocramius’s stale review January 11, 2017 10:06

Requested changes applied

@lcobucci
Copy link
Member Author

@lcobucci no, 2.6, as this actually changes cache prefixes, and may lead to deployment issues.

@Ocramius alright thanks!

@Ocramius Ocramius assigned Ocramius and unassigned lcobucci Jan 11, 2017
@Ocramius
Copy link
Member

LGTM 👍

@Ocramius Ocramius merged commit 4e304df into doctrine:master Jan 11, 2017
@lcobucci lcobucci deleted the fix-l2c-region-namespace branch January 11, 2017 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants