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

Delayed reading from the persistent storage until actually needed #85

Closed
wants to merge 1 commit into from

Conversation

BenMorel
Copy link

@BenMorel BenMorel commented Aug 3, 2013

The current implementation of BaseFacebook::__construct() reads a value from the persistent storage.

This might not be a problem with the default Facebook class, which relies on the PHP session storage, which is always available.

However, having subclassed BaseFacebook myself to support a homebrew persistent storage system (we don't use PHP's $_SESSION), I ran into the following problems:

  • When I instantiate (actually, when my Dependency Injection framework instantiates) a service that has a dependency on Facebook, a variable is read from the storage, even if no subsequent methods are called on the Facebook class. This is a waste of resources.
  • When I run my tests, or CLI tools, sometimes I also need an instante of a Service that has a dependency on Facebook, even though I will never actually use something that makes a call to the Facebook class. In my CLI tools, no session is started, and therefore Facebook makes everything fail.

This PR fixes this, by delaying the read of the state variable until it is actually needed.

@gfosco
Copy link
Contributor

gfosco commented Oct 15, 2013

Sorry it took this long to respond to this. Thanks for your contribution, but at this time we do not wish to add this functionality to the core SDK.

@gfosco gfosco closed this Oct 15, 2013
@BenMorel
Copy link
Author

@gfosco Is there a specific reason why? There is absolutely no BC break here.

@gfosco
Copy link
Contributor

gfosco commented Oct 15, 2013

The scope of the change is not insignificant, and would be difficult to test, which is not included with the PR. There is not a significant demand for this feature and it would increase our surface area for maintenance. We have quite a few plans for enhancing this project and making it PHP-FIG standards compliant, once we can work through the backlog of requests and contributions. It's possible that after we get to that point, we may be more open to this change.

@BenMorel
Copy link
Author

Thanks for your answer. Actually, for the next major version I think the storage should be an interface, that should be passed to the Facebook constructor, which is a much clear approach IMO than having to subclass a BaseFacebook. I've detailed my thoughts in this wishlist a few months ago:

https://developers.facebook.com/bugs/612436278789603

A new version of the Facebook SDK whould be great anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants