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 Config::Cookie Class #4508

Merged
merged 1 commit into from
Apr 10, 2021

Conversation

mostafakhudair
Copy link
Contributor

While we are taking a step forward to make config folder separated as much as possible, Cookie class should have it's own config file

@mostafakhudair mostafakhudair force-pushed the config-cookie branch 4 times, most recently from 926ac46 to 105302e Compare March 30, 2021 21:04
@MGatner
Copy link
Member

MGatner commented Apr 1, 2021

This is a good change. I would like to make a release this weekend, which would solidify all the Cookie changes since 4.1 - let's be sure to get this finished so it can be included.

@mostafakhudair
Copy link
Contributor Author

@MGatner I think next release should be done without any of new Cookie Class, because of cookie class is bloated with alot usless methods and need to be refactored, also to avoid BC

@MGatner
Copy link
Member

MGatner commented Apr 4, 2021

@mostafakhudair I thought there had been significant improvements to our Cookie handling since the last release, work that you and @paulbalandan both did. What do you think should be held back? Is it something that can easily be reverted and re-applied for the following release?

Paul, what are your thoughts?

@mostafakhudair mostafakhudair force-pushed the config-cookie branch 2 times, most recently from 6d90b06 to 42bdb15 Compare April 4, 2021 22:25
@paulbalandan
Copy link
Member

Bloated? What aspects of the Cookie are bloated? Cookie is just implementing its contract interfaces' methods, which are actually getters and cloners of a web cookie's attributes. Plus, it has bonus methods to help parse a Cookie-Header string into a functional Cookie object, and some validation methods to make sure the passed attributes are valid against the multitude of cookie specifications. Are these useless to you? The bloat, I would say IMO, is the ArrayAccess implementation and some ?: logic, but that is a compromise to retain backward compatibility to the time cookie handling was just using plain arrays.

If you can pinpoint the places that needs refactor, we'd consider excluding cookie in the release and perhaps I'd do the refactor myself. Otherwise, aside from the config reassignment, the cookie class is stable as-is.

system/Cookie/Cookie.php Outdated Show resolved Hide resolved
system/Cookie/Cookie.php Outdated Show resolved Hide resolved
@samsonasik
Copy link
Member

@mostafakhudair please incorporate @paulbalandan review

app/Config/Cookie.php Outdated Show resolved Hide resolved
system/Cookie/Cookie.php Outdated Show resolved Hide resolved
system/Cookie/Cookie.php Outdated Show resolved Hide resolved
system/Session/Session.php Outdated Show resolved Hide resolved
system/Test/Mock/MockCookieConfig.php Outdated Show resolved Hide resolved
tests/system/Cookie/CookieTest.php Outdated Show resolved Hide resolved
tests/system/Security/SecurityTest.php Outdated Show resolved Hide resolved
@mostafakhudair mostafakhudair force-pushed the config-cookie branch 2 times, most recently from 21ab076 to 216b095 Compare April 6, 2021 18:18
@MGatner MGatner mentioned this pull request Apr 8, 2021
@mostafakhudair mostafakhudair force-pushed the config-cookie branch 6 times, most recently from 82a4907 to 6f17f45 Compare April 9, 2021 17:57
@mostafakhudair
Copy link
Contributor Author

What is wrong with tests?

@samsonasik
Copy link
Member

That seems race condition issue in SQLSRV action. It seems unrelated.

@MGatner
Copy link
Member

MGatner commented Apr 10, 2021

Have tests been rerun since #4539?

@mostafakhudair
Copy link
Contributor Author

Yes, twice

@paulbalandan paulbalandan merged commit 4841326 into codeigniter4:develop Apr 10, 2021
@paulbalandan
Copy link
Member

Thank you, @mostafakhudair

@mostafakhudair mostafakhudair deleted the config-cookie branch April 10, 2021 16:04
@MGatner
Copy link
Member

MGatner commented Apr 10, 2021

🥳

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

Successfully merging this pull request may close these issues.

4 participants