Problem/Motivation
The \Drupal\project_browser\ProjectBrowser\Project class comprises an army of getters and setters that really just access private properties.
Now that PHP supports typed, readonly properties, I'm not sure we really need this stuff. I would suggest we replace as many getters and setters as possible with public, typed properties. We could even use constructor property promotion, which would allow calls like this:
new Project(
isCompatible: true,
isMaintained: true,
isActive: true,
projectUsageTotal: 39,
);
...lots of flexibility is available to us. And we can remove a lot of boilerplate.
The outstanding questions:
Should some (or all) of these properties also be readonly? Is there any reason to expect that code will want to -- or even should -- mutate these properties? ANSWER: There may or may not be reason to mutate them, but let's keep them mutable for now to keep the current behavior intact. Making them read-only has backwards compatibility implications.
Should any of the properties remain private on purpose, mediated by getters and setters? I'd imagine that such a thing would only be necessary for properties which need a little extra special handling. ANSWER: Only setSummary() needed this treatment, since it has actual logic. The other properties didn't, so they are good candidates for just becoming public.
Comments
Comment #4
srishtiiee commentedComment #5
phenaproximaI think this looks really good. Also, check out that diffstat:
That's nearly 400 lines removed! NICE!!
The only thing I'm unsure about here is the use of
readonly-- that's changing the behavior of the Project class, and to me it should not be done in this issue.But otherwise this looks RTBC to me.
Comment #6
srishtiiee commentedComment #7
phenaproximaI'm really sorry about this, but I noticed something about setting the summary -- if there's some logic around setting a property, then we probably don't want that property to be public.
Otherwise this looks great.
Comment #8
srishtiiee commentedAdded the setSummary() method back 👍🏼
Comment #9
phenaproximaShip it!
Comment #10
phenaproximaComment #11
phenaproximaComment #12
tim.plunkettSaving credit
Comment #14
tim.plunkettMerged! Thanks