8000 [RFC][Session] Native storage handlers should not update the global config by vicb · Pull Request #4208 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][Session] Native storage handlers should not update the global config #4208

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
Closed
wants to merge 1 commit into from
Closed

[RFC][Session] Native storage handlers should not update the global config #4208

wants to merge 1 commit into from

Conversation

vicb
Copy link
Contributor
@vicb vicb commented May 5, 2012

I submit this as a RFC because we can not get to an agreement on things should be done with @Drak and I would like to hear the voice of the community on this point.

Memcache will be used as an example for the discussion.

Context

When you want to use memcache to store your sesssion data, you will need to create an instance of "NativeMemcacheSessionHandler" and pass the save_path as argument. The save_path specify the host, port and a few options (i.e. the ones passed to addServer).

All the other options (i.e. the compression factor, the serializer, ...) are retrieved from the ini values.

What we do today

In order to configure the native memcache storage the NativeMemcacheSessionHandler class update the ini values.

What I propose

I propose not to update any ini value that is gobal to all the memcache instances. To me ini values are global configs that should not be updated for a local need.
(I am ok with updating session specific settings ('memcache.session_redundancy') because those are not shared by all the memcache instances)

In order to configure the memcache native storage:

  • update your php.ini with the desired value,
  • or if you don't have access to the php.ini (i.e. on a shared server) set the value at some "global" place (i.e. your kernel).

The question

Do you think:

A- It is ok to change any ini settings from inside the Session ? (The current state)
B- We should not modify any ini settings that is not strictly related to the session from inside the session. (The state after this PR)

@fabpot
Copy link
Member
fabpot commented May 7, 2012

With the information provided in this PR, I would say B. But I want to hear @Drak point of view before taking a decision.

@fabpot fabpot mentioned this pull request May 7, 2012
Closed
@ghost
Copy link
ghost commented May 7, 2012

@fabpot - I need time to make a reply - will be busy for two days.

@stof
Copy link
Member
stof commented May 7, 2012

I would also say B

@ghost
Copy link
ghost commented May 11, 2012

Sorry, but this has turned into a long answer :)

background

The 'memcache' extension provides three things to PHP. It provides some memcache_*() functions, the Memcache object, and a native session save handler. (native) 'save handler' here means save handlers provided by PHP extensions (not our Native*Handler classes) - they are internal to PHP with no external API/interface to configure or interact with them.

The Memcache class is a simple object which provides a API to any memcache server. Each memcache instance can be configured by it's methods. The 'memcache' save handler provides the internal callbacks for PHP internally to read and write the session data. The method of configuring this save handler is via ini_set('memcache.*'). All native save handlers are configured by ini_set() - even if it's just session.save_path. Although PHP ini settings are global variables they are the only way to configure native save handlers since there is no public API for them.

The Native*Handler classes simply make configuration of native save handlers uniform across the Symfony2 session API and make things a little more OOP, with the understanding that we are still using globals under the hood. The whole Sessions code with all the bags and so forth are ultimately just manipulating a superglobal. Ultimately it's ugly, but unavoidable since there is no other way to interact with PHP sessions. It's made clean by a uniform API.

In reality, to use native save handlers, you don't actually need to even use Native*Handler classes at all. `ini_set('session.save_handler', 'memcache/sqlite/memcached/redis') will do the same thing - it makes it more arcane, and drops the OOP paradigm and making thing less organised

overview

The basis of this PR is an assumption there are ini directives that are session specific and those which are not. This is not really the case since they are all used to configure native save handlers - the only way you can configure them since they are part of PHP's internals.

analysis

The perceived issue is the PHP Memcache extension Memcache class has a "bad" API: specifically Memcache::connect($server, [$port]) and addServer($server, [$port]). If $port is unspecified it will take the default value from ini memcache.default_port. Since these two methods defaults are unpredictable, the developer should explicitly set the port. It's a matter of documentation. Memcached does not suffer from this.

Ini values do not affect existing configured Memcache object instances.

The intention of the memcache.default_port directive related to defaults in the session.save_path which used to configure the native save handler's memcache server(s). Anyone using memcache has to be aware of this behaviour, it's a matter of documentation.

The Native*Handlers make it really easy when using HttpFoundation as a standalone component to configure and use php native save handlers and it makes no sense to neuter these classes so they cannot configure the very save handlers they activate (in an OO way).

Imagine how ugly it would be if you say, "here's how to setup native memcache session, but you might additionally have to set some runtime ini-values". We cannot decide what we think is "session related" since all the ini values for each extension are the method of configuring the native handlers.

Please note, Memcached does not suffer this problem as Memcache::addServer() requires$hostand$port`.

solution

While it is true that some default behaviour is affected by ini directives, they just affect unspecified defaults when invoking certain methods.. I have updated the documentation for Memcache::connect() and Memcache::addServer() to clarify this. We can have some extra notes in our docs/docblocks too. See: http://docs.php.net/memcache.addserver and http://docs.php.net/memcache.connect (will render after the cronjobs run later today at 12:15 UTC).

I would also note that all the code examples at php.net already specify explicit method invokation of addserverand connect() with both host and port.

conclusion

I would prefer documentation. We should not neuter the classes' ability to perform - especially since it does not protect against anything anyway: Globals are globals. Let's not punish Sf2's classes for a badly designed PHP extension.

The same is true for the NativeSessionStorage class which also allows configuration of ini values, removing those just gets rid of convenience, it doesn't protect the developer and brings must more inconvenience.

Please note in both cases, configuration is optional and the classes do not change the global environment with default values except in one case session.cache_limiter

On the plus side, the ability to configure the classes this way allows convenience like being able to configure them via Symfony2 DIC in the Sf2 full-stack framework.

I would really prefer if we treat this as a documentation issue.

@snc
Copy link
Contributor
snc commented May 11, 2012

I don't like setting global ini settings by ini_set() and I already talked about it in IRC... my solution is simple, I don't use the Native* classes and just configure my php.ini and I'm done.

I never benchmarked it but I believe that calling ini_set() some times takes longer than not calling it at all :-)

Proper documentation like @Drak suggested could be an alternative to B.

@vicb
Copy link
Contributor Author
vicb commented May 11, 2012

@Drak thank you very much for your extensive feedback, below are some comments (I don't expect your reply on those, they are here for clarification - I know will we not agree)

background

The method of configuring this save handler is via ini_set('memcache.*')

You probably mean "The method of configuring this save handler is via ini settings."

In reality, to use native save handlers, you don't actually need to even use Native*Handler classes at all. ini_set('session.save_handler', 'memcache/sqlite/memcached/redis') will do the same thing.

I think this is a bit out of scope of this PR but it is worth discussing a little. What will happen in case you have session.auto_start set to 1 ? I think a session will be started even before you get a chance to configure anything. That is the session is started using the php.ini values (what will happen if they are wrong, ie you try to access a memcache server at a wrong IP ?) and those settings are modified afterward - we still have an open discussion on this topic.

overview

What I meant is that some settings (eg memcached.sess_locking, memcached.sess_lock_wait) are session specific. While some other (memcache.chunk_size, memcache.default_port, memcache.hash_strategy, sqlite.assoc_case) affect both the session storage and any other instance of the underlying class.

As session handling is centralized in the Session "component" the first type should be safe to modify as nobody else will ever be impacted, the second type is not.

analysis

Since these two methods defaults are unpredictable, the developer should explicitly set the port. It's a matter of documentation.

I don't understand your reasoning here: why doesn't it apply to the session storage as well. As you have the ability to set the port in the save_path when using the memcache/d save handler (tcp://host2:11211), you should also set it explicitly and it will remove the need to have to ini_set('memcache.default_port', ...).

The default port might be a bad example as it could explicitly set both when using the session storage and connecting to a server for a memcache/d instance.

memcache.chunk_size could be a better example as there is no other way to set the value other than ini settings. I see no reason why setting this value should be handled by the Session "component" rather than at a global level (in the php.ini file).

Imagine how ugly it would be if you say, "here's how to setup native memcache session, but you might additionally have to set some runtime ini-values".

I can imagine, but I find also ugly to set global values from inside the Session "component". This PR is about finding the best compromise.

solution

While it is true that some default behaviour is affected by ini directives, they just affect unspecified defaults when invoking certain methods

I don't agree, see memcache.chunk_size above, sqlite.assoc_case is yet an other example.

@vicb vicb mentioned this pull request May 11, 2012
Closed
@ghost
Copy link
ghost commented May 11, 2012

I can imagine, but I find also ugly to set global values from inside the Session "component". This PR is about finding the best compromise.

The compromise is already there - none of the configuration is compulsory. It put 100% control in the developers' hands. Even down to session.save_path you can defer to the php.ini values by setting $path=null in the constructor. If these classes were loading up defaults and making assumptions, I would agree, but they are not, they simply give the option. I might add, in the beginning they were, and there was discussion about it in FrameworkBundle related issues and the code was adjusted to make everything optional.

@ghost
Copy link
ghost commented May 15, 2012

I have recently confirmed the intention of the ini directives with the memcached team and indeed, the ini directive are targeted just as I have explained - to configure the native handlers. For this reason, and all the reasons explained already, this PR should be closed. I understand the motivation behind this PR, but it is unfortunately, not appropriate in this context.

Please also see the reference in #4255 (comment) which explains why session.auto_start is also out of the scope of this PR (and has been addressed in #4264

@vicb
Copy link
Contributor Author
vicb commented May 15, 2012

I have recently confirmed the intention of the ini directives with the memcached team and indeed, the ini directive are targeted just as I have explained - to configure the native handlers.

This PR is not in contradiction with your statement. Using this PR, the native handlers would be configured by the ini values set in the php.ini file (as opposed to ini_set()).

With this PR auto_start = 1 could be used for native handlers (but not for custom handler) as explained in 4255.

@vicb
Copy link
Contributor Author
vicb commented May 30, 2012

Native storages have been removed in #4454

@vicb vicb closed this May 30, 2012
@antych
Copy link
antych commented Jun 11, 2012

I think there's an issue with this change. Having memcache save_path in a config file is very useful. If you add a new server to your pool, you can modify the list in one place and release code. Keeping it in php.ini would mean logging in to X production servers and restarting apache on each one. There are some obvious workarounds for it, but IMO this makes native handler much less useful.

@ghost
Copy link
ghost commented Jun 11, 2012

@antych - I actually agree with you, but it saying that it's just as easy to create your own native driver class. I may re-release those drivers in a bundle if I get time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vicb @fabpot @stof @snc @antych
0