Move flagging integrity checks from service to Flagging::save() and unflagging checks to a better place.

save() is definitely the wrong place, you need to use pre or postSave.

save() is just a shortcut for calling $storage->save($flagging), then your code wouldn't run.

I am triggering a retest because the errors I get locally are just different from from those listed in testbot's report.

Also I think things may have changed a bit see #2657384: Flagging should use some standard entity reference

I am posting a patch with functionally identical code

the entity manger is deprecated ...and another pending issue is removing all references to it .. and so I am just smoothing out any potential conflicts.

Just proving to myself what is wrong ... I would be the first to admit that my change is clumsy

But before I fix it properly I want to open a side issue

From Flagging:baseFieldDefinition() both

$fields['flagged_entity'] and $fields['uid'] should set the cardinality to 1 as it does not make sense for there to be more than one.

But that is a change I would like to see in an isolated issue.

In #9 I introduced a clumsy way of getting at the user behind $this->uid

So I am fixing that ... the tests should be better now that #2669994: Failing tests has been committed.

Fixing the tests is trivial .....

Now that the flagging verification tests are run in preSave() .. and because preSave position in the call stack they are rethrown as EntityStorageExceptions
by SqlContentEntityStorage::save() hence the test errors.

But before I supply a patch I want establish a good place to run our integrity test .. As if differs for flagging and unflagging and will result in a significant refactoring of unflagging.

A summary of flagging MkII ( FlagService::flag() )

1) Flagging::create()
2) Flagging::save() which triggers integrity checks in preSave()
3) dispatch FlaggingEvent

So far so good..

The difference is that the unflagging event must precede Flagging::delete() so the event can make use of the data in the existing flagging.
Consequently I want to move all the integrity checks from FlagService::unflag() and FlagService::getFlaggging() into FlagService::getFlaggings()
That way all the checks are common when ever a flagging is requested.

so the summary of FlagService::unflag() :-

1) FlagService::getFlagging() -- triggers integrity checks
2) Flagging::delete()
3) Dispatch Unflagging event.

Anyway I will get to this soon..

Ah yes sorry .. thanks for pointing out my err.

Changed the title to reflect that fact that unflagging integrity checks are now in a place where they will be of more general use.

as per #13 I fixed tests and refactored unflagging logic.
The only deviation is that I decided not to move logic from FlagService::getFlaggging() into getFlaggings()

I have fixed up the documentation so I had to added preSave to the interface.

Looking at FlagServiceTest - There is a design outcome/kink which is a natural outcome of what I described in #13.

flagging integrity checks are rethrown and need to be caught as a EntityStorageException
unflagging integrity checks are not rethrown and come out as logic Exceptions.

locally FlagServiceTest now passes. but now FlagService::getFlagging is more exacting in what it accepts other tests might fall over.

From #14

Oh that's less than ideal.

Messages from Flagging::presave() like

'The flag does not apply to entities of this type.'

do filter up - but it is true that the stack trace becomes fragmented, taking more detective work to figure it out than it should.

There are other directions to try, not without issues of their own, an access control handler which rejects requests before entry into the controller?

At the moment I have socket wenches "review all the things" mantra in my head.

I am trying a new approach.

What is relevant from https://drupalwatchdog.com/volume-5/issue-2/introducing-drupal-8s-entity...
( see the subsection "Creating Your Own Constraint Plugins" )
is the implied instruction "Copy what you need from Comment, CommentNameConstraint and CommentNameContraintValidator"

So I copied the classes and converted them for our purposes. this is a very early version so we can discuss the pros and cons

Pros
1) It abstracts the validation into a location inherently tied to the Flagging entity so it can easily be called whenever we want in future code.
2) No more inappropriate EntityStorageExceptions breaking the stack trace.

Cons
1) Complexity - OMG all the extra boiler plate!
2) I have not yet added all the code to loop through and dealing with any generated violations
3) It covers only flagging not unflagging.

Some variable names are too long, I am looking at you FlagEntityUserConstraint::messageEntityTypeFlagTypeMismatch