Closed pine3ree closed 5 years ago
@pine3ree I'm a bit worried about exposing the HTTP_DATE_FORMAT
constant, can we just inline the format (implementation + test)? I'm fine with merging this but if you're willing to make that change, that would be great 🚀
@hansott ,
imo nothing to worry about...being a constant it can only be read, not written to. If/when you update version and requirements to php ^7.1 you can also add private
visibility to it. But of course I'll do what you prefer, it's your package :-). We can also use a private static variable (in the test cases as well to avoid repetition in code), but its nature it's indeed the nature of a constant string. Same goes for the cookie-name-forbidden-chars regex constant I introduced in my other PR. I'll wait for your decision. kind regards..
Thanks for your patience. I always try to keep public API as small as possible. When it's public we should assume people are using it. If we add this class constant, people might depend on it in their code and if we would remove it again, their code will break (for PHP this will be a runtime error, unless they are using a linter of course). Then again, I'm being paranoid. I love your efforts and I want you to have a positive experience with open-source! It's up to you. 🙌 (I prefer if we hardcore the format in the class + test)
@hansott Not paranoid at all, I had the same doubt and I agree. I'll change the code and just suggest to use a private constant when the min php version requirement gets switched to 7.1. I will change the code in the second PR as well. Do you prefer a private
(full encapsulation/isolation) or a protected
(so that it could be referred/used in consumer code descendants as well) static variable to hold the value? Or just the inline value? kind regards.
Inline the format seems a good solution to me! Thanks man!
@hansott I inlined the value, but left a private static variable (plus commentary) in the test class to avoid re-typing the format in the test cases. I will update/rebase my other PR, and probably split it into more than 1 based on the purpose, so that the commit history stays clean. kind regarsds.
https://github.com/hansott/psr7-cookies/issues/9