Skip to content

Conversation

@lolautruche
Copy link
Contributor

Based on #177, ref #170

This PR adds 2 options that one can add in their getOptions():

  • fos_native_subscribers: bit options to activate/deactivate subscribers provided by FOSHttpCacheBundle

    By default all native subscribers are activated (HttpCache::SUBSCRIBER_ALL).

Examples with fos_native_subscribers

ALL native subscribers EXCEPT the user context one

protected function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_ALL | ~self::SUBSCRIBER_USER_CONTEXT
    ]
}

NO native subscribers

protected function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_NONE
    ]
}

ONLY user context subscribers

protected function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_USER_CONTEXT
    ]
}

User context subscriber AND Foo subscriber

protected function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_USER_CONTEXT | self::SUBSCRIBER_FOO
    ]
}

HttpCache.php Outdated
Copy link
Member

Choose a reason for hiding this comment

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

does it need to be public ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Did it for testing purpose, but I could use Reflection in the test.

Copy link
Member

Choose a reason for hiding this comment

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

Use Reflection then. Turning a private method into a public one just for testing purpose is bad, because public methods are affected by BC while private are not.

@dbu
Copy link
Contributor

dbu commented Dec 2, 2014

thanks, looks good. i will add your PR doc into the .rst doc, once we merged this. so we need to figure out the options / init question.

@lolautruche
Copy link
Contributor Author

Hmmm, it seems that conflicts appeared suddenly... Tell me when you're finished with modifications in your branch :-)

@lolautruche
Copy link
Contributor Author

Rebased on your branch @dbu (solved conflicts that appeared after your last update).
So repeating my arguments on getOptions() vs constructor change:

  • More elegant (IMHO).
  • Easier to use.
  • Doesn't need to modify the front controller.

Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer to remove this option and only keep the bitmap.

creating those subscribers should be done outside the kernel, or can be done in the constructor. there is no benefit in the indirection that i can see.

Copy link
Contributor

Choose a reason for hiding this comment

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

or the user can overwrite getSubscribers to add his own subscribers, instead of the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll remove the option then. Documentation should state that one can add custom subscribers by overriding getSubscribers().

@dbu
Copy link
Contributor

dbu commented Dec 3, 2014

i updated my branch from the feedback from stof. i will leave it alone now until this is merged, to avoid further conflicts :-)

can we "compromise" on using the options bitmap only for default subscribers, and leave it to the user to do custom subscribers in getSubscribers or the constructor, or the frontend controller?

Based on FriendsOfSymfony#177, ref FriendsOfSymfony#170

This PR adds 1 option that one can add in their `getOptions()`:

* `fos_native_subscribers`: bit options to activate/deactivate subscribers provided by FOSHttpCacheBundle

> By default **all native subscribers are activated** (`HttpCache::SUBSCRIBER_ALL`).

**ALL native subscribers EXCEPT the user context one**
```php
public function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_ALL | ~self::SUBSCRIBER_USER_CONTEXT
    ]
}
```

**NO native subscribers**
```php
public function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_NONE
    ]
}
```

**ONLY user context subscribers**
```php
public function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_USER_CONTEXT
    ]
}
```

**User context subscriber AND Foo subscriber**
```php
public function getOptions()
{
    return [
        'fos_native_subscribers' => self::SUBSCRIBER_USER_CONTEXT | self::SUBSCRIBER_FOO
    ]
}
```
@lolautruche
Copy link
Contributor Author

@dbu Just updated 😄 . I removed the custom subscribers option.
Custom subscribers can then be added by overriding getSubscribers(), constructor or by editing the frontend controller 😃

I also fixed a fatal error due to namespace collision from your last refactoring in respect to feedback from @stof

dbu added a commit that referenced this pull request Dec 3, 2014
Added subscriber options to HttpCache
@dbu dbu merged commit 1193cd8 into FriendsOfSymfony:refactor-symfony-cache Dec 3, 2014
@dbu
Copy link
Contributor

dbu commented Dec 3, 2014

great, thanks a lot! i merged it into my branch so that we can discuss in one place again :-)

@lolautruche lolautruche deleted the subscriberOptions branch December 3, 2014 15:07
@lolautruche
Copy link
Contributor Author

So cool ! Thanks for merging 👍

@dbu
Copy link
Contributor

dbu commented Dec 3, 2014

thanks for the contribution!

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.

3 participants