Skip to content
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

Create new Storage component to reduce code duplication between components #2089

Merged
merged 3 commits into from
Oct 31, 2014

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Oct 31, 2014

Next step after #2088.

@arnolanglade
Copy link
Contributor

Insteresting! is some classes (like Sylius\Component\Storage\SessionStorage) are missing ?

@stloyd
Copy link
Contributor Author

stloyd commented Oct 31, 2014

@arnolanglade arnolanglade added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Oct 31, 2014
@stloyd
Copy link
Contributor Author

stloyd commented Oct 31, 2014

@pjedrzejewski I'm not sure I should also adjust Cart to have same approach as we have for Currency & Locale components...

@pjedrzejewski
Copy link
Member

@stloyd Nice, I like it. Regarding cart, any reasons against?

@saidul
Copy link
Contributor

saidul commented Oct 31, 2014

Does it mean to allow storing cart id in cookie?

@stloyd
Copy link
Contributor Author

stloyd commented Oct 31, 2014

@pjedrzejewski I guess that would be quite big BC break ;)
@saidul Yes, that was the point I had in mind.

@koemeet
Copy link
Contributor

koemeet commented Oct 31, 2014

I think its a good idea to do the same with the Cart. See issue #2083. I think if you store it in a session or cookie, we could have this working, right?

@stloyd
Copy link
Contributor Author

stloyd commented Oct 31, 2014

@steffenbrem Already pushed changes for Cart too.

@koemeet
Copy link
Contributor

koemeet commented Oct 31, 2014

@stloyd Ah I see, great 👍 This is the best approach IMO.

pjedrzejewski pushed a commit that referenced this pull request Oct 31, 2014
Create new Storage component to reduce code duplication between components
@pjedrzejewski pjedrzejewski merged commit d2dff71 into Sylius:master Oct 31, 2014
@pjedrzejewski
Copy link
Member

Thanks Joseph! 👍

@stloyd stloyd deleted the feature/storage branch November 1, 2014 07:40
@koemeet
Copy link
Contributor

koemeet commented Nov 1, 2014

@pjedrzejewski This package is not available on packagist yet, this means we cannot install the resource-bundle etc. seperately. Even if I add the Storage repository to my composer.json, it won't install it. Does this PR has anything to do with it?

In the composer.json of the Storage component, it says its package name is: sylius/sequence?

@koemeet
Copy link
Contributor

koemeet commented Nov 6, 2014

@stloyd @pjedrzejewski The cookie storage does not work correctly. I think because the cookie is only set on the request with $this->request->cookies->set($key, $value); and never on the response. So this is currently broken. I think you need a listener and add the cookie to the response later on, using: $response->headers->setCookie().

Also, in the storage key a dot (.) sign is used, which will be replaced by PHP. So even if the cookies did work, the keys would not match and the cart (or anyother stored thing) could not be fetched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants