Closed craue closed 8 years ago
I think the BC Break on the StorageInterface
regarding to return the removed data is fine. If someone wants to read the data upfront, you can still retrieve it before removing the entry. As you are going to put it into 3.0, that's fine by semver, too.
I would change the UserIdProviderInterface
into something like StorageKeyGeneratorInterface
/StorageKeyProviderInterface
:
namespace Craue\FormFlowBundle\Storage;
interface StorageKeyGeneratorInterface
{
/**
* Generates a complete storage key based on the key the storage received.
*
* @see StorageInterface
*
* @return string
*/
public function generate($key);
}
This removes the pseudo-dependency (per its name) that keys are required to be bound on a user.
Furthermore the DoctrineStorageUserIdProvider
may be renamed to a more generic one, as it has nothing to do with Doctrine by itself: SessionKeyGenerator
, UserSessionKeyProvider
, ...
@havvg, done.
:+1:
For the record, the 2nd commit now introduces the mentioned BC break.
@craue What is the right way to use DoctrineStorage. I was hoping to set it up for "finish later" flow (#152). Right now, I am unable to get the database entries to clear out - does that need to be done explicitly by calling "remove()"?
I didn't manage to find any documentation of how to configure the Doctrine Storage (does it exist anywhere?). However, after looking at the code, I configured/created the following services and it worked (I didn't test it thoroughly though).
app.form.flow.user_session_storage_key:
class: Craue\FormFlowBundle\Storage\UserSessionStorageKeyGenerator
arguments: ['@security.token_storage', '@session']
app.form.flow.storage.doctrine:
class: Craue\FormFlowBundle\Storage\DoctrineStorage
public: false
arguments: ['@doctrine.dbal.default_connection', '@app.form.flow.user_session_storage_key']
app.form.flow.data_manager:
class: Craue\FormFlowBundle\Storage\DataManager
public: false
arguments: ['@app.form.flow.storage.doctrine']
app.form.flow:
class: %craue.form.flow.class%
calls:
- [setDataManager, ['@app.form.flow.data_manager']]
- [setFormFactory, ['@form.factory']]
- [setRequestStack, ['@request_stack']]
- [setEventDispatcher, ['@?event_dispatcher']]
@craue is there a simpler way?
This will help people switching from session-based to Doctrine-based storage.
I wonder if it's worth breaking BC by changing
StorageInterface
to let theremove
method not return the removed value anymore. Returning it was easy forSessionStorage
, but it would require an additional query to fetch the value before removing it for the newDoctrineStorage
. To avoid that, I'd like to change the interface.Also, instead of adding a new Travis job to run the suite with SQLite, I'd prefer running all integration tests twice for each job, first with parameter
db.driver
set tonull
(for default session storage) and then set topdo_sqlite
(for Doctrine storage) in order to cover both setups in each environment. Is this possible?I'd love to get feedback, especially from @havvg.