Change FlagService::flag() and ::unflag() to use objects to discourage use of IDs

I don't think we need the (un)flagbyID() methods at all. D7 was already encouraging the use of $flag->flag() rather than the completely ID-based flag_flag(). On D8, a $flag is easily loaded via core APIs, so it's not even as though there's an API to learn for getting hold of a flag object.

Postponing on #2461549: use entity type hinting in routes

When I try and makes these changes without that patch is just comes out a tangled mess.

Yeah, definitely.

There's also the matter of moving these to the Flag entity class.

But I think we should do this refactoring here. Discussing whether we move the code can be a follow-on.

While I would instinctively include this method in the Flag entity

I have to say both paths seem equally valid....

Rattling around in my brain is a talk given by Crell - where he creates a Hug service and deliberately makes the Hug entity is a stripped down minimal thing.

If you don't mind - I am going against my instincts and going to produce a Fat Service - Skinny Entity patch.

If for nothing else but to see how it looks.

Ok, well let's leave that discussion to a future issue, and here just refactor things in place :)

It does sound like there are good arguments on both sides and I look forward to it!

Found a bug: unflagByObject() doesn't decrease the flag counts. Postponing for this. #2466253: unflagByObject() doesn't decrease the flag counts.

Also, I'm confused about this code:

    $flaggings = $this->getFlaggings($entity, $flag, $account);
    foreach ($flaggings as $flagging) {
      $out[] = $flagging->id();

      $this->unflagByFlagging($flagging);

For a trio of entity, flag account, there can be only one flagging entity! Anyway, one for a separate clean-up issue later on.

This is now active :)

This would probably need a change record. I see we've not been writing them for 8.x anyway, so a general "This is how you flag stuff on D8" change record should be started now, to cover prior (and future) changes from the old D7 API.

@martin107: are you still wanting to work on this?

Yes I hope to give a patch tonight -- in say 2 hours :)

Ok - this is the first pass....

What guided my approach

- "Just keep things in the service for now and see how it looks."
- "Just remove to flag/unflag methods we don't need."

Where appropriate I have continued the work of using type hinting in controller methods to utilize some existing 404 error handling.

I did some manual testing and ran 4 tests locally ... but I expect I may have missed some corner cases... but it is good enough to show to testbot.

This is a blocker --
I will try and jump on any failures tonight -- but I have other things todo Friday --- I can do some more work over the weekend.

rats

Cool. I was going to take it if you didn't have time to look at this one -- hope it didn't come across as me pestering!!

Your patch goes a bit too far though... remove a few things and we're good to go!

+++ b/src/Controller/AJAXLinkController.php
@@ -25,20 +25,17 @@ class AJAXLinkController extends ControllerBase {
-  public function flag(FlagInterface $flag, $entity_id) {
-    $flag_id = $flag->id();
+  public function flag(FlagInterface $flag, EntityInterface $entity) {

That's not going to work without a corresponding change to the route in the routing.yml. It's the subject of another issue, and there are several possible ways to do it (including leaving it alone!): #2465505: use the EntityConverter to load the flaggable entity in the routes.

You'll need to load the entity before calling flag() -- use getFlaggableById() on the service.

As for the pestering ... no problem my way of signalling that this was my current focus was to unassign myself from the other issues on the roadmap that I was working on.... which upon reflection is about as helpful as an ashtray on a motorcycle. :)

Thanks for stopping me blindly clashing with #2465505: use the EntityConverter to load the flaggable entity in the routes

FlagSimpleTest now passes.

+++ b/src/Controller/AJAXLinkController.php
@@ -51,21 +49,18 @@ class AJAXLinkController extends ControllerBase {
    */
-  public function unflag(FlagInterface $flag, $entity_id) {
-    $flag_id = $flag->id();
+  public function unflag(FlagInterface $flag, $entity) {

I'm afraid you've missed one :( This is a param change in a controller. (Test isn't picking it up because 1. we don't have AJAX controller tests IIRC, and 2. our tests don't all cover unflagging).

Ok -- I will have more time to look at this Saturday :(

No problem. Here's a that change.

I think this issue is now complete...

Here is a draft change record.
https://www.drupal.org/node/2468957

I am just re-uploading the last patch, ONLY so that the status can be moved to needs review.

Um, there was already https://www.drupal.org/node/2468957.

And we definitely don't need 2 CRs. We really only should have one CR that covers the whole of the evolution of 'how you flag stuff' from 7.x-3.x to 8.x-4.x.

Is this active because we have 2 CR's. I am not sure...

If so socketwench's is fine and published and looks complete.... mine is only a 'draft'

do you want me to delete mine? it not a problem

Yup, I set it back to NW because we have 2 CRs, and either one of them needs work, I'm afraid.

I think we should write and organize our CRs with the intended audience of site builders and module developers who are upgrading from 7-3 to 8-4. So a single CR should tell you how to get from D7 style procedural flag_flag($flag_name, $entity_id) to the service method with entity parameters. Developers shouldn't have to read through several CRs that cover different steps that retrace the development history of Flag's 8-4 branch!

Those are fair points about the difficulties of CRs; however, writing a monolithic developer guide would be quite literally a step backwards. It was because single developer guides from one major version to another were completely unmaintainable that CRs were devised for d.org.

CRs do have problems of their own. They are way too many of them for core, and they're often not written very well (e.g., I once found a CR about the removal of a hook that didn't give the name of the hook in the title!), or with much of an awareness of the whole body of documentation that they form together.

Despite that, I think they're a much better way of communicating changes, provided we consider that together they should form the developer guide, instead of seeing them as one-shot consequences of closing an issue.

With that in mind, we should have one CR that documents changes to 'so you want to flag or unflag something programmatically?'

I can have a shot at writing what I have in mind.

Yeah, this is one area where CRs are weak (but then again, monolithic documentation would be lousy here too).

I believe the vast majority of our audience will be developers in the future wanting to upgrade their site or their custom module or their contrib module from D7 to D8. They simply don't care that in the process of getting to a stable 8.x-4.x, Flag went via FlagService::flag($flag_id, $entity_id, $user). Making them read two CRs that take their code to this intermediary step would be like some awful coding version of recapitulation theory!

In terms of numbers, our 8.x-4.x-dev release has 33 installs. I reinstall D8 core quite regularly, so I don't know if that counts as multiple installs to d.org. Meanwhile, over 20k users are on a stable 7.x-3.N release. Granted, not all of them need to use the API or have any need of this CR.

> We would have to trash the CR I've already written, and it would possibly mean editing previous CRs if a D8 to D8 change like this occurs.

I only see 2 CRs for 8.x: the one you published for this issue, and one I wrote.

> when a D8 to D8 change does occur, we need to create an additional CR to cover that change independently. If there's a way to link changes together, we should try to do that as much as possible.

We don't have any means on d.org to indicate that CR A is a subsidiary of CR B, and that if you're looking at B, you can safely ignore A. It's not a bad idea, and you should suggest it to the core docs maintainer, whom I believe shares some of my concerns about how CRs have turned into an information overload that's hard to get through. I think the biggest problem with CRs though is cultural though :(

Done.

I've taken the liberty of removing the bits in the CR that described development history and motivation, as I feel these are superfluous. Imagine a developer who in a year's time is tasked with upgrading a D7 site: what do they need to know? Do they care about what we decided and how, or the intermediate steps we took? No; all they want to know is what to change in their code.

And yup, if we change the name of the FlagService class in future, that CR will need tweaking. That does make it a smidge harder for devs who are following us closely, but if you're following development that closely, you can probably read the git log...

Going to say this is fixed now.

I happened to spot today a core CR that said at the top of it 'This is a D8-to-D8 change record'. So it looks like core is going for option 2 in #33 above. If someone wants to write a D8-to-D8 change record for this issue, I'm fine with that, but I think we have so few developers affected by this that I don't know if it's worth it.