{"action":"created","issue":{"url":"https://api.github.com/repos/cakephp/cakephp/issues/7954","labels_url":"https://api.github.com/repos/cakephp/cakephp/issues/7954/labels{/name}","comments_url":"https://api.github.com/repos/cakephp/cakephp/issues/7954/comments","events_url":"https://api.github.com/repos/cakephp/cakephp/issues/7954/events","html_url":"https://github.com/cakephp/cakephp/issues/7954","id":124683713,"number":7954,"title":"RFC 3.3/4.0: Using separated getter/setters","user":{"login":"dereuromark","id":39854,"avatar_url":"https://avatars.githubusercontent.com/u/39854?v=3","gravatar_id":"","url":"https://api.github.com/users/dereuromark","html_url":"https://github.com/dereuromark","followers_url":"https://api.github.com/users/dereuromark/followers","following_url":"https://api.github.com/users/dereuromark/following{/other_user}","gists_url":"https://api.github.com/users/dereuromark/gists{/gist_id}","starred_url":"https://api.github.com/users/dereuromark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dereuromark/subscriptions","organizations_url":"https://api.github.com/users/dereuromark/orgs","repos_url":"https://api.github.com/users/dereuromark/repos","events_url":"https://api.github.com/users/dereuromark/events{/privacy}","received_events_url":"https://api.github.com/users/dereuromark/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/cakephp/cakephp/labels/Enhancement","name":"Enhancement","color":"bfe5bf"},{"url":"https://api.github.com/repos/cakephp/cakephp/labels/RFC","name":"RFC","color":"c7def8"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/cakephp/cakephp/milestones/69","html_url":"https://github.com/cakephp/cakephp/milestones/3.3.0","labels_url":"https://api.github.com/repos/cakephp/cakephp/milestones/69/labels","id":1443324,"number":69,"title":"3.3.0","description":"A backwards compatible feature release for 3.x. See the [3.3 Roadmap](https://github.com/cakephp/cakephp/wiki/3.3-Roadmap)","creator":{"login":"markstory","id":24086,"avatar_url":"https://avatars.githubusercontent.com/u/24086?v=3","gravatar_id":"","url":"https://api.github.com/users/markstory","html_url":"https://github.com/markstory","followers_url":"https://api.github.com/users/markstory/followers","following_url":"https://api.github.com/users/markstory/following{/other_user}","gists_url":"https://api.github.com/users/markstory/gists{/gist_id}","starred_url":"https://api.github.com/users/markstory/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/markstory/subscriptions","organizations_url":"https://api.github.com/users/markstory/orgs","repos_url":"https://api.github.com/users/markstory/repos","events_url":"https://api.github.com/users/markstory/events{/privacy}","received_events_url":"https://api.github.com/users/markstory/received_events","type":"User","site_admin":false},"open_issues":3,"closed_issues":0,"state":"open","created_at":"2015-12-04T02:24:56Z","updated_at":"2016-01-04T01:25:37Z","due_on":"2016-06-12T04:00:00Z","closed_at":null},"comments":21,"created_at":"2016-01-04T01:25:37Z","updated_at":"2016-01-18T14:59:03Z","closed_at":null,"body":"Currently we use those combined setters/getters everywhere basically.\r\nThose have three large drawbacks IMO:\r\nThey\r\n- complicate the logic inside a method, having to separate the get vs set cases\r\n- Make null setting impossible\r\n- Don't go in the direction PHP goes in general, with PHP7 and stricter return types (currently always at least two: value for get and $this etc for set)\r\n\r\nAnd one of the more dangerous things with the current way:\r\nChaining can blow up (Setters can accidently become getters resulting in fatal errors).\r\nI have seen this myself a few times and seen it reported by people:\r\n\r\n    $this->setterOne($value)->setterTwo(...)->...\r\n\r\nNow if $value for some reason becomes null (sure, it shouldnt), it will fatal error as it setterOne(null)=getterOne() and therefore returning maybe a scalar value which breaks the chain hard.\r\n\r\nWe are also one of the only code bases that actually follow this combined API approach afaik.\r\nIf we want to simplify the API, extracting those into separate methods makes most sense.\r\nIt solves the above issues by design and is also more clear in its usage, using the prefixes:\r\n```\r\nfoo() -> getFoo()\r\nfoo($x) => setFoo($x)\r\n```\r\nMethods are cheap and should be rather doubled as a result than possible mistakes with the internal get/set switching.\r\n\r\nOften times we have super complicated stuff with 2nd or 3rd arguments then, because depending on the first they behave differently or not. This would be trivial with separation as those are then only applicable for the respective method.\r\n\r\nIt also allows us in the future to more easily adapt PHP7 return type hinting without having to break API everywhere. Reducing the number of possible return types should be a priority for a clean method API in 3.x to clean all that up then in 4.x as stated in the [roadmap](https://github.com/cakephp/cakephp/wiki/4.x-Roadmap).\r\nPlease note that this roadmap also already contains a note about the issues of the current API and that this is not future proof as it stands.\r\n\r\nWe can also start deprecating and using the separated setters/getters in 3.x and then remove those in 4.x.\r\nThat would ease the migration.\r\n\r\nMost of those separations can also be automated with the upgrade app tooling I imagine.\r\nIf there is no argument it must be a setter, if there is a argument (and not null assumed) it must be a getter.\r\nI will write a sniffer/fixer to check on any return type that is more than a single one and we can check why that is the case. \r\nEven `SomeObject|null` should be avoided, as null is not a valid secondary return type anymore in PHP7 strict terms, but that is a different topic alltogether (especially regarding object getters returning null fallbacks).\r\n\r\n//EDIT: To ease migration we could also keep those deprecated combined ones in 4.x while recommending not to use them anymore. There is no need to directly throw them over board except for the maintenance overhead. But since we are not adding return typehinting any time soon (for PHP5 compatibility) there will not be any issue if we decide to keep them a while longer. \r\nThe only important thing is that people can start to write code the *new* way asap to avoid having to refactor later. All those who want to postpone know that and can live with the consequences.\r\nBut in 5.0 we definitely should remove them then."},"comment":{"url":"https://api.github.com/repos/cakephp/cakephp/issues/comments/172552486","html_url":"https://github.com/cakephp/cakephp/issues/7954#issuecomment-172552486","issue_url":"https://api.github.com/repos/cakephp/cakephp/issues/7954","id":172552486,"user":{"login":"hmic","id":876917,"avatar_url":"https://avatars.githubusercontent.com/u/876917?v=3","gravatar_id":"","url":"https://api.github.com/users/hmic","html_url":"https://github.com/hmic","followers_url":"https://api.github.com/users/hmic/followers","following_url":"https://api.github.com/users/hmic/following{/other_user}","gists_url":"https://api.github.com/users/hmic/gists{/gist_id}","starred_url":"https://api.github.com/users/hmic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hmic/subscriptions","organizations_url":"https://api.github.com/users/hmic/orgs","repos_url":"https://api.github.com/users/hmic/repos","events_url":"https://api.github.com/users/hmic/events{/privacy}","received_events_url":"https://api.github.com/users/hmic/received_events","type":"User","site_admin":false},"created_at":"2016-01-18T14:59:03Z","updated_at":"2016-01-18T14:59:03Z","body":"OK, so you want to get rid of them, why 5.0 not 4.0 and mark them deprecated in 3.3 already? Introducing the new, not BC breaking split getters/setters in 3.3 too?\r\n\r\nOne thing I do want a split setter to do is returning the set value, like the combined ones do now too :p\r\nAny complains about this behavior?\r\nAlternatively they should at least return true/false indicating if the value has been set"}}