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

Remove session from factorydefault or start it when creating instance #12921

Closed
Jurigag opened this Issue Jun 21, 2017 · 27 comments

Comments

Projects
6 participants
@Jurigag
Copy link
Member

Jurigag commented Jun 21, 2017

Right now having session service in Phalcon\Di\FactoryDefault is pointless, because you need to start session instance anyway to make it working properly. So you need to have code like this anyway:

$di->setShared('session', function() {
    $session= new Phalcon\Session\Adapter\Files();
    $session->start(); // this is required pretty much to make session things working properly, including persistent
    return $session
});

If you don't have this and you are using just session from FactoryDefault then it won't work properly because it's not started until you will start it by accessing this service and executing start method. Like value after doing $session->set() will be not available right now when using session from FactoryDefault in other request.

Or we could change it to just start session on instance construction.

This is somehow related issue - #12918

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 21, 2017

Actually, as you can see, we just use "connect" approach in many adapters if they does not connected yet. So what do you think about check session status? IMO we can remove session from factory default in 4.x branch only.

@Jurigag

This comment has been minimized.

Copy link
Member Author

Jurigag commented Jun 21, 2017

To make it working and as a bugfix for now for phalcon 3 - fine. But in phalcon 4 to avoid overhead better just remove it from factory default.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 21, 2017

@Jurigag

This comment has been minimized.

Copy link
Member Author

Jurigag commented Jun 21, 2017

Also i would consider thinking about for example memcache adapters and other - do we really need to do

let memcache = this->_memcache;

if typeof memcache != "object" {
	this->_connect();
	let memcache = this->_memcache;
}

In every single method? Why just not call _connect one time in __construct?

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 21, 2017

Refs: #10530

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

If we do leave the session adapter in the factory web DI then it should be setup in a way that allows it do be loaded from a static YML or INI configuration file for the most simple files setup.

I'd like to make it so that simple builtin plugins could be added from the configuration file under the plugins key. For example now I'm adding a database reconnect plugin and it would be great if I could hook it in using the same yml file. This is along the lines of not just ease of use and clarity but using the "perch" tool so that it could ask "Would you like to enable this plugin?..." (more or less).

So I bring this up because neither the existing convention nor this new proposal would exactly work. What if we added an interface to classes that are meant to be used as services so that the DI could check if they are started and if not then autostart it by calling the interface defined start()?

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

@Jurigag Is there a crazy performance penalty on checking if an object implements an interface? I ask because sometimes PHP surprises me.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

So lets say that you want to add plugins before any events have been fired then you could still do that and if for some reason you need to hack in something that requires the service to be started (whatever that means for it) then you could do that in the service definition file for advanced cases. Just the DI would ensure that the service was started before it returned it for the first time.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

Off of the top of my head StatefulServiceInterface.

@niden

This comment has been minimized.

Copy link
Member

niden commented Jun 22, 2017

Removing it from 4.x is the best option.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

Personally I'm tired of seeing the following:

$di->set('service', function() {
    $obj= new Phalcon\Class();
    $obj->setDI($this);
    $obj->start();
    return $obj;
});

$di->set('service', function() {
    $obj= new Phalcon\Class();
    $obj->setDI($this);
    return $obj;
});

I think that we could just remove it from the factory DI in the 4.x tree early on and then see where the rest of development takes us. I think that we could provide a flavor of DI that sets things up automatically according to adherence with just a few interfaces. For example if it is DiInjectable then it could automatically add the DI if it doesn't have one. If it is StatefulServiceInterface then it ensures that it has started. I think that this will help out immensely with the new tooling that we are imaging and creating now in the shadows.

So yes lets just go ahead and remove our problem edge cases and then if there is a will to do it we can reevaluate it later. Maybe its a bad idea, maybe its a good idea but more of a Phalcon 5 thing.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

If we do something along the lines that I am proposing then the following becomes viable and solid for the very simple cases.

$di->set('session', Phalcon\Session\Adapter\Files::class);

Also:

session: Phalcon\Session\Adapter\Files
@Jurigag

This comment has been minimized.

Copy link
Member Author

Jurigag commented Jun 22, 2017

@dschissler well i think you can have calls like call method also when setting service in di in some other syntax right? So loading from some format is good idea. Im just talking about session service, not other things - that right now, current one in FactoryDefault is useless - because you need to START session anyway, and to start it you need to call session method yourself, so most people will just set session service themself or you could get existing one from FactoryDefault and just call start method. Other idea is to just start session after creating class.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Jun 22, 2017

@Jurigag Well for the moment I think that we have 3-0 (not sure if sergeyklay voted) to just remove it. So I think that we should do that. The next step is all of this fancy stuff that I'm talking about and that may or may not happen. So lets just remove it and then see what else falls into place later. Its a different discussion.

@micheleangioni

This comment has been minimized.

Copy link

micheleangioni commented Aug 14, 2017

Hi,
IMHO after almost 2 months the Phalcon Team should take a decision, not leaving this issue open since it's quite important.

Is this the expected behavior, which will cause breaking a lot of Phalcon projects upon updating to 3.2 without BC notice in documentation, or Phalcon should start a session by itself on instance construction?

I would prefer the second option, with Phalcon starting a session on instance construction, however let's just take a decision and fix or close this issue :)

Thanks! :)

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Aug 14, 2017

This would not affect the 3.* branch so it is not a pressing issue.

@micheleangioni

This comment has been minimized.

Copy link

micheleangioni commented Aug 14, 2017

Really? So an issue introduced in 3.2 will be fixed in 4.0? That's at least weird, it should be fixed in 3.2.x if the current behavior is not what it's intended to be.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Aug 14, 2017

semver

@micheleangioni

This comment has been minimized.

Copy link

micheleangioni commented Aug 14, 2017

Semver has been broken introducing a BC from 3.1 to 3.2, which is forbidden by semver. IF we want to FIX this, 3.2.x should be used to revert the BC.
THEN, if we want to confirm the BC because we think it's needed (I would rather confirm the fix, i.e. starting a session on instance construction), it should be introduced in 4.0.

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Aug 14, 2017

Which specific thing was broken in 3.2? Its not supposed to happen but I guess that it does sometimes. It doesn't happen a lot at least.

@micheleangioni

This comment has been minimized.

Copy link

micheleangioni commented Aug 14, 2017

See #12918 , I got the same fatal error as I updated Phalcon from 3.1 to 3.2 in a small side Project.
I agree with Jurigag's opinion (if you are not starting a PHP session then it's not good and it's ok that an error arises), but still we have a BC since there was no error in 3.1. The proposed solution (starting a session on instance construction) seems to me to be the best thing to do in 3.2.x to fix the BC.

About 4.0 we should think whether remove the session start on instance construction. IMHO, I would leave that also in 4.0.

@virgofx

This comment has been minimized.

Copy link
Member

virgofx commented Aug 14, 2017

Vote for pure removal in Phalcon v4

@sergeyklay sergeyklay added this to the 4.0.0 milestone Aug 17, 2017

@Jurigag

This comment has been minimized.

Copy link
Member Author

Jurigag commented Aug 28, 2017

@micheleangioni well actually it wasn't due to 3.1 > 3.2 really, it was change in zephir which is causing this error. Right now zephie checks if certain superglobal exists, and if not you have error like this. I think it's better to remove session service in phalcon 4, first people will need to choose their adapter and second they will need to actually start it.

@micheleangioni

This comment has been minimized.

Copy link

micheleangioni commented Sep 1, 2017

@Jurigag ok, understood. It seems a honest approach.

@sergeyklay sergeyklay added RFC and removed Requires-Discussion labels Sep 4, 2017

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Nov 8, 2017

I think that we should solve this in a generic way so that other services can take advantage of it.

Currently done in Phalcon\DI::get to inject the DI:

		/**
		 * Pass the DI itself if the instance implements \Phalcon\Di\InjectionAwareInterface
		 */
		if typeof instance == "object" {
			if instance instanceof InjectionAwareInterface {
				instance->setDI(this);
			}
		}

Why can't we use an approach like this to start a service object if it implements an interface and isn't already started? So you can manually start it in the closure but if you don't then it will ensure that it has been started before handing it off.

This would work great for classes which have basic setups while still allowing them to be customized in the closure. This will handle our super basic session files service as well as many others.

@Jurigag

This comment has been minimized.

Copy link
Member Author

Jurigag commented Dec 19, 2017

@dschissler i think this idea with StatefulServiceInterface is cool, so any class impelemting when resolving service will start it. @sergeyklay

@stale stale bot added the stale label Apr 16, 2018

@sergeyklay sergeyklay closed this Apr 16, 2018

@sergeyklay sergeyklay reopened this May 2, 2018

@stale stale bot removed the stale label May 2, 2018

@Jurigag Jurigag referenced this issue Oct 19, 2018

Merged

Reorganized/cleaned up Session classes #13538

2 of 3 tasks complete

@niden niden added this to In progress in 4.0 Release Nov 28, 2018

@niden niden moved this from In progress to To do in 4.0 Release Nov 28, 2018

@phalcon phalcon deleted a comment from stale bot Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

@niden niden referenced this issue Dec 17, 2018

Merged

T12921 remove session from di factory #13667

2 of 3 tasks complete

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

@niden niden moved this from To do to In progress in 4.0 Release Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 18, 2018

[phalcon#12921] - Merge branch '4.0.x' into T12921-remove-session-fro…
…m-di-factory

* 4.0.x:
  [phalcon#13061] - Updated changelog
  [phalcon#13061] - Added test for the interface
  [phalcon#13061] - Aligned interfaces
  [phalcon#13639] - Updated the changelog
  [phalcon#13639] - Corrected tests
  [phalcon#13639] - Corrected tests
  [phalcon#13639] - Renamed Acl\Role to Acl\Operation
  [phalcon#13639] - Renamed Acl\Resource to Acl\Subject

niden added a commit to niden/cphalcon that referenced this issue Dec 18, 2018

niden added a commit that referenced this issue Dec 18, 2018

Merge branch 'niden-T12921-remove-session-from-di-factory' into 4.0.x
* niden-T12921-remove-session-from-di-factory:
  [#12921] - Changes to the changelog; corrected test
  [#12921] - Corrected tests
  [#12921] - Added issue to the changelog entry
  [#12921] - Updated changelog
  [#12921] - Removed session from DI\FactoryDefault; Minor cosmetic changes

@niden niden moved this from In progress to Done in 4.0 Release Dec 18, 2018

@niden

This comment has been minimized.

Copy link
Member

niden commented Dec 22, 2018

Addressed

@niden niden closed this Dec 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment