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

I implemented a straightforward cookie collection mechanism #66

Closed
wants to merge 11 commits into from

Conversation

hacan359
Copy link

In my project, I needed to work with cookies from requests in different parts of the application. I couldn't find a ready-made solution, so I created my own locally. It's a simple collection that is populated in middleware and can be used from anywhere in the code, along with a straightforward implementation of a provider for the collection.

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues comma-separated list of tickets # fixed by the PR, if any

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like this PR.

Suggest move all new files to RequestCookies folder. One feature - one directory.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ceba1d0) to head (71b9b6b).
Report is 6 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##              master       #66   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       132       146   +14     
===========================================
  Files              5         9    +4     
  Lines            338       366   +28     
===========================================
+ Hits             338       366   +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +11 to +13
RequestCookiesProviderInterface::class => [
'class' => RequestCookiesProvider::class,
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RequestCookiesProviderInterface::class => [
'class' => RequestCookiesProvider::class,
],
RequestCookiesProviderInterface::class => RequestCookiesProvider::class,


declare(strict_types=1);

namespace Yiisoft\Cookies\RequestCookies\Exception;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace Yiisoft\Cookies\RequestCookies\Exception;
namespace Yiisoft\Cookies\RequestCookies;

use Throwable;

/**
* Thrown when Request cookie collect isn't set before.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Thrown when Request cookie collect isn't set before.
* Thrown when request cookies isn't set before.

{
public function __construct(int $code = 0, ?Throwable $previous = null)
{
parent::__construct('Request cookie collect is not set.', $code, $previous);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parent::__construct('Request cookie collect is not set.', $code, $previous);
parent::__construct('Request cookies is not set.', $code, $previous);

Comment on lines +37 to +38
*
* @see getValue()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*
* @see getValue()

/**
* @var RequestCookies|null The collection.
*/
private ?RequestCookies $cookieCollection = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private ?RequestCookies $cookieCollection = null;
private ?RequestCookies $cookies = null;

*
* @param RequestCookies $cookieCollection The collection to set.
*/
public function set(RequestCookies $cookieCollection): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function set(RequestCookies $cookieCollection): void;
public function set(RequestCookies $cookies): void;

Comment on lines +27 to +28
$cookies = $this->collectCookies($request);
$this->cookieProvider->set(new RequestCookies($cookies));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$cookies = $this->collectCookies($request);
$this->cookieProvider->set(new RequestCookies($cookies));
$this->cookieProvider->set(new RequestCookies($request->getCookieParams()));

Seems, getCookieParams always contains array<string,string>, isn't he?

@@ -146,6 +146,46 @@ $cookie = (new \Yiisoft\Cookies\Cookie('cookieName'))
->withRawValue('ebaKUq90PhiHck_MR7st-E1SxhbYWiTsLo82mCTbNuAh7rgflx5LVsYfJJseyQCrODuVcJkTSYhm1WKte-l5lQ==')
```

Work with cookie request collection from any place in you project. Add in config you app in middleware block
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Work with cookie request collection from any place in you project. Add in config you app in middleware block
### Request cookies collection
You can work with cookie request collection from any place in you project. Add in config you app in middleware block

}
}
```
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add line to changelog

@vjik vjik added the status:code review The pull request needs review. label Oct 26, 2024
@vjik vjik requested a review from a team October 26, 2024 09:39
if (!is_string($name) || !is_string($value)) {
continue;
}
$collection[$name] = $value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I access expires, domain and other parameters later?

/**
* Get the current request cookie collection.
*
* @throws RequestCookieCollectionNotSetException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd return an empty collection instead

/**
* Provides a way to set the current cookie collection and then get it when needed.
*/
interface RequestCookiesProviderInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CookiesProvider sounds better than RequestCookiesProvider

*/
private ?RequestCookies $cookieCollection = null;

public function set(RequestCookies $cookieCollection): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have an empty collection in this class and merge two collections instead.
Moreover it can be used in two ways:
The first is as you did
The second is the similar that error-handler has: set parameters in the app and add them back to request in a post-middleware later. Wdyt?

@vjik
Copy link
Member

vjik commented Oct 26, 2024

Closed in favor yiisoft/request-provider#11

@vjik vjik closed this Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants