Problem/Motivation

Let's add Dependency Injection and split up the classes.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3543241

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:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan opened merge request !13091

nicxvan’s picture

nicxvan commented
Status: Active » Needs review
mstrelan’s picture

mstrelan commented

Looks fine to me, only question is if we've given up on single hook classes with __invoke methods, like \Drupal\file\Hook\CronHook? Otherwise we could do this for cron and help here, even theme if we don't intend to use it for anything else.

nicxvan’s picture

nicxvan commented

I'm not really a fan of that approach to be honest.

nicxvan’s picture

nicxvan commented
Title: Modernize Announcements Feed Hook implementations. » Modernize Announcements Feed Hook implementations
Parent issue: » #3493453: [meta] Standardize and clean up hook classes in core
smustgrave’s picture

smustgrave commented

Question from my phpstorm. Since all the parameters are being marked readonly phpstorm wants to mark the class readonly too. Is that valid?

nicxvan’s picture

nicxvan commented

I have no idea the implications of that, I would need to read up.

smustgrave’s picture

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

Probably not a blocker here. Updates LGTM.

mstrelan’s picture

mstrelan commented

Yeah I've considered the same. If the class is readonly then all properties are readonly, including those of child classes. I believe that would be fine, but given there is pushback for having final classes then I guess we shouldn't restrict what they can do either?

smustgrave’s picture

smustgrave commented

Was more curious, it was a recommendation IDE was making not sure where it pulls it from

mstrelan’s picture

mstrelan commented

If the php language level in phpstorm is 8.2 or above (when readonly classes were introduced) and all properties on the class are marked readonly, then it will suggest that the class can be readonly.

astonvictor’s picture

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

I left one comment. Does it make sense?

smustgrave’s picture

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

No it’s not needed with the hooks

nicxvan’s picture

nicxvan commented

It's actually all constructors that don't need comments, not just ones on hooks: https://www.drupal.org/docs/develop/standards/php/api-documentation-and-...

astonvictor’s picture

astonvictor commented

ok, thanks for the reply

I just saw that almost all constructor methods in the core contain comments e.g. Constructs a {Class name} object..

So, it's like optional but recommended.

mstrelan’s picture

mstrelan commented

So, it's like optional but recommended.

More like it's legacy from a time before typed parameters and properties.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I think readonly classes vs. individual readonly properties (when all of them are readonly) might be a coding standards question - in that we should choose one or the other.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • catch committed 02eea477 on 11.x 
    Issue #3543241 by nicxvan, mstrelan: Modernize Announcements Feed Hook...

catch closed merge request !13091

Status: Fixed » Closed (fixed)

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