Refactor count API into FlagCountService

First draft ... it may need more than a little finessing - but I hope the general shape is correct.
I don't think it yet warrants serious review but I prefer to publish early.

So to start off just a comment
The constants FlagEvent::ENTITY_FLAGGED/ENTITY_UNFLAGGED/FLAG_DELETE

This is fine and dandy... but given the event name the $action parameter of the FlagginEvent::_construct is redundant/removed.

  /**
   * Builds a new FlaggingEvent.
   *
   * @param \Drupal\flag\FlagInterface $flag
   *   The flag.
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity to be acted upon.
   * @param string $action
   *   The action to perform. One of 'flag' or 'unflag'.
   */
  public function __construct(FlagInterface $flag, EntityInterface $entity, $action) {

I have added @event tags to the names of the events as per the coding standard.
https://www.drupal.org/coding-standards/docs#event

In the fullness of time they need the appropriate @see tags to link to the new classes.

The FlaggingCounterSubscriber is little more than a wrapper for 2 methods in the flag.count service ... which use calls to a Drupal::service
Ugh so not great maybe the flag.count service is a little redundant?

Anyway more tomorrow

I kept this postponed because I knew the events were not firing and FlagConfirmFormTest was failing

Sorry for burying the lead - it is work in progress ... registering the service was the missing key ... so thank you for the prompt reply a little nudge in the right direction - and thanks for solving hidden bugs :)

For access to Entity and Flags from the Event I have opted to add getters... others may want to convert the property from private to public
Meh ... me I have have no opinion and will be flexible.

Naming thing is harder though . I am trying to fit in with the implicit conventions of the modules.

I have renamed FlaggingCountSubscriber() to FlagCountSubscriber()

We have a db table flag_counts.
FlagEvents for the act of Flagging
A service flag.count

In that context is seemed correct.

I still think it is early days and I am having a mini-review myself. but I am moving to needs review to trigger testbot.

1) Splitting off changes to FlagEvents as an out of scope side issue.
#2471086: Standardise annotations surrounding FlagEvents

2) getters -- now annotated using a standard description prefered in a drupal context.

3) A handful of minor coding standard tweeks.

I am happy to leave this issue as a "just move code into the correct place" thing... and leave my structural comments to the other issues pent-up behind this one.

In that light I think it looks complete.

I contend some API change is justified.

I have reworked the code to remove all changes to FlagEventBase.php - by placing getFlag() in FlaggingEvent.php

This is because not all future classes extending FlagEventBase will need to grant public read access to the $flag, but FlagCounterSubscriber requires it.

Some API changes hived off into

#2473349: Expose FlaggingEvent::flag/entity for public read access.
#2473423: Remove redundant FlaggingEnity::action

making this issue postponed on them

2) Fixed.

3) Corrected.

When the other issue are commit this patch will have to be revisited to fix issue related to #2473423: Remove redundant FlaggingEnity::action

As part of the discussion do I understand you correctly

Do you want the flexibility to access these functions from outside of the event system.

Say to allow for the possibility when triggering a fully blown FlaggingEvent has too much overhead - or to run this procedurally?

\Drupal::service('flag.count')->incrementFlagCounts(FlagInterface, $flag, EntityInterface $entity) 

This is a reroll triggered by #2473423: Remove redundant FlaggingEnity::action

Locally I see FlagSimpleTest fails... but I think that is broken in head at the moment.

Drupal core is changed alot today as DevDays ends.

It seems odd to me to name the class FlagCountEventSubscriber AND expose that as a service. It would make more sense to me to name it FlagCountService since to a contrib developer working with flag, they would care more that it's a service than how it does it's job internally (as an event subscriber).

There should also be an interface for the FlagCountsService in the case third party modules want to replace it. The same should go for FlagService, but that's another issue.

From the issue summary

Refactor and fix the flag count API into a new FlagCountService. Instead of being tied directly to the FlagService, FlagCountService would rely on the event system and listen to the flag and unflag events.

But not all FlagCountService function need to listen to flag and unflag events.

Regarding the name change. It would be reasonable for me to be out voted 2 to 1 on this but before I move the method associated with the FlagEvent into a FlagCountService - let me do the uncontroversial thing and move the other five count methods from flag.module into a newly created FlagCountService/FlagCountServiceInterface

I note these methods are deprecated and from flag core they are only called in FlagCountsTest.

It was a bit more than a copy past exercise - I tried to find some reasonable name changes....

I am just including text, that maybe coming to a change record near you soon :)

flag_get_counts($entity_type, $entity_id) becomes \Drupal:service('Flag.count')->getFlagCounts($entity_type, $entity_id)

flag_get_entity_flag_counts($flag, $entity_type) becomes \Drupal:service('Flag.count')->getEntityFlagCounts($flag, $entity_type)

flag_get_flag_counts($flag_name, $reset = FALSE) becomes \Drupal:service('Flag.count')->getFlagTotalCounts($flag_name, $reset = FALSE)

flag_get_user_flag_counts($flag, $user) becomes \Drupal:service('flag.count')->getUserFlagCounts($flag, $user)

All these function use db_select which in the next iteration should be directly injected into the service.
and phpcs reports lots of documentation error in FlagCountServiceInterface but I want a small step copy and paste stage as much as possible.

[ The FlagCountSubscriber methods also use db_merge where a testable dependency injection solution should be found. ]

It would make more sense to me to name it FlagCountService since to a contrib developer working with flag, they would care more that it's a service than how it does it's job internally (as an event subscriber).

I was following the almost universally followed core convention that things that do "implements EventSubscriberInterface" have a subscriber suffix. So we pull developers in 2 directions.

With feedback from both of you I will move

incrementFlagCounts
decrementFlagCounts(FlaggingEvent $event)
getSubscribedEvents()

into the FlagCountService

But this is my last attempt to draw a distinction between event methods and stand alone service calls.

Sure I will make the changes.

Given that Berdir commented on another issue that most core service classes aren't called FooService, and that we could consider renaming FlagService, I can see a case for a unified FlagCountManager.

Agreed.

1) Merged FlagCountService and the EventSubscriber into FlagCountManager and its corresponding FlagCountMangerInterface.

2) Now directly inserting the database service into the FlagCountManager, removing calls the db_select, db_update and db_remove.
The next step on the meta is add more testing ... so I want leave the new class in a state ready for that.

3) As for the "phpcs reports lots of documentation error in the interface" I have just cut and pasted the original text -- -so there is no change in documentation quality
I will add this as a separate issue to the meta....on the basis it is easier to review tightly focused issues.

Manually tested a simple flag -- could unflag and flag ok
FlagCountTest passes locally - let us see what testbot makes of the rest of it.

Liking what I'm seeing so far. Minor quibbles on method names and if increment and decrement can be protected rather than public, but I'm willing to let that slide so we can get this patch in. I'll try to do a more thorough review later.

Sure naming things is hard.... I will be happy to make any changes that you like....

If I remember correctly those two methods need to be public as something deep within the bowls of Mordor err I mean the event system takes up the string name from the array returned by FlagCountManager::getSubscribedEvents().

I will be happy to make any changes that you like....

Mostly having so many somthingFlagsomething() methods seems silly to me. We know its for Flag. It's on the FlagCountManager.

If I remember correctly those two methods need to be public as something deep within the bowls of Mordor err I mean the event system

Looks like. Pity. Still, good to keep it off of the interface.

I would go for method names more like this.

new names look better, draft look good.

side issue created.

Unless I am missing something this issue looks complete...

I have made a slow visual scan of the changes....

Mentally I followed the movement of the comment and the method into their new homes...
I don't think anything has slipped...all looks good.

so +1 from me. I will work on the change record this afternoon ( in about 3 hours. )

Here is the draft change record.

https://www.drupal.org/node/2478089