On a few events we have public attributes that event subscribers can modify to alter behavior. We've been back and forth on using this pattern of public properties vs method calls that wrap the property. I haven't had a strong opinion either way until now, where I am leaning towards using method calls:

1. We have complete control over the data that actually gets set on the property by using a setter/getter. So someone won't be able to set a boolean property to a string or something.
2. The API is a bit more obvious, especially in cases where we are already using getters/setters for other properties but not all of them.

CommentFileSizeAuthor
#5 method-calls-for-api-2830620.5.patch11.11 KBbkosborne
#2 method-calls-for-api-2830620.patch11.02 KBbkosborne

Comments

bkosborne created an issue. See original summary.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Active » Needs review
StatusFileSize
new method-calls-for-api-2830620.patch11.02 KB
metzlerd’s picture

metzlerd commented 
+++ b/src/CasRedirectData.php
@@ -147,6 +147,30 @@ class CasRedirectData {
+  public function setCacheable() {

I'm not sure about this pattern. Forcing people to call different setters for a different value seems like it will complicate the implementation code. Any reason why we don't use setCacheable(TRUE) and setCacheable(FALSE)?

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs review » Needs work

Hmm I see your point. I'll update with a single setter.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs work » Needs review
StatusFileSize
new method-calls-for-api-2830620.5.patch11.11 KB

I changed the setters/getters as suggested. Sorry, I screwed things up a bit and wasn't able to generate an interdiff =/

metzlerd’s picture

metzlerd commented
Status: Needs review » Reviewed & tested by the community

No problem, Thanks!

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Ugh, I never actually committed this!