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.
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

phenaproxima created an issue. See original summary.

srishtiiee made their first commit to this issue’s fork.

srishtiiee’s picture

Status: Active » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

I think this looks really good. Also, check out that diffstat:

+ 201
− 568

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.

srishtiiee’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

I'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.

srishtiiee’s picture

Status: Needs work » Needs review

Added the setSummary() method back 👍🏼

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Ship it!

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes
tim.plunkett’s picture

Saving credit

tim.plunkett’s picture

Status: Reviewed & tested by the community » Fixed

Merged! Thanks

Status: Fixed » Closed (fixed)

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