Create FlagTestBase to house common functions

Looking like a good start!

1. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +
   

   Surplus blank line.

2. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +   *  Optional. The entity type of the flag to create. If NULL, assumes 'node'.
   

   Should be '(optional)'.

   Also, I'm not entirely sure about special-casing node here. Smells a bit D6! ;)

3. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +   *  A new flag with the given criteria.
   

   Let's say 'flag entity' to be clear.

4. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +    $flag->save();
   +
   +    return $flag;
   

   It's best to have a sanity-checking assertion here, so in case something disastrous goes wrong, we can see where the problem is.

5. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +  /**
   +   * @param string $entity_type
   +   * @param array $edit
   +   * @param string $link_type
   +   *
   +   * @rerturns
   

   Needs a bit of a tidy-up...

6. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +    // Submit the flag add/edit form.
   

   Aren't add and edit different forms now? And here we're only ever creating.

7. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +  protected function getFlagType($entity_type) {
   

   Doesn't most of our module code assume that the plugin ID and the entity type ID are identical? I should check that...

8. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,220 @@
   +  protected function getBundleIds($entity_type) {
   

   Do we need this?

   This could be inlined as array_keys(entity_get_bundles()).

Now where is my copy of Flash Gordon ... I haven't see that terrible movie in a long time.
Oh wait is was hiding behind the film of George Clooney driving the batman franchise into a hole for a decade....

Nothing of any significance, swept some coding standard errors out of FlagTestBase.php

Plus now using $this->t() in preference over pulling t() from global state...

will try and find time from a proper review tomorrow.

> Yeah... The thing is we do test mostly nodes right now.

Yes, I know -- I'm feeling conflicted about the special-casing, as I can see a case for it too! :/

The patch looks like a good refinement to me to so +1

one minor quibble

The name createFlagForm() fools me into thinking its job is to create a form.

So I have change it to createFlagViaForm --

naming things is hard ... sorry if I am treading on anyone's toes...

> The name createFlagForm() fools me into thinking its job is to create a form.

Seems like a good idea to me. I can't speak for socketwench's toes of course, but were it me, my toes would be feeling unharmed and, indeed, appreciative of the input :)

Dyslexics of the world, Untie!

createFlagFromForm() - there is letter order issue for dyslexics that might misread it as a repeated word.

Via - That issue also occurred to me, so if two people have spotted it then it is out.

How about

createFlagByForm()
createFlagWithForm()

?

We're officially bikeshedding now... ;)

I prefer createFlagWithForm. 'by form' sounds oddly formal, like trial by combat or something.

Here's what I hope is a final nitpick:

1. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,244 @@
   +   *   (optional) The entity type of the flag to create. If NULL, assumes 'node'.
   +   * @param array $subtypes
   +   *   (optional) An array of entity subtypes to which the flag applies. If NULL,
   

   Wrapping needs fixing here.

2. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,244 @@
   +      'flag_long' => $this->randomString(16),
   ...
   +      'flag_short' => $this->randomString(16),
   +      'flag_long' => $this->randomString(16),
   

   This is a change from the previous functionality, and I'm really not keen on these being completely random, because it will make debugging tests harder. When you look at the pages seen by a test, you won't be able to tell whether a link is for flagging or unflagging. So can we have them as 'Flag this item' instead?

3. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,244 @@
   +   * If you do not provide any bundles in $edit, all bundles for $entity_type
   +   * is assumed.
   

   'all bundles... is'?

   Also, could this be reworded so it doesn't use the 2nd person?

4. +++ b/src/Tests/FlagTestBase.php
   @@ -0,0 +1,244 @@
   +   *   (optional) A string containing the flaggable entity type.
   

   Needs to say the default is 'node'.

Also, I still think we don't need getBundleIds().

Ugh, this has rotted. I'll try and reroll.

Reroll, with in addition the disabling of strict config checking removed in the new base class, as that's been removed everywhere else since the patch in #17 was posted.

If the testbot likes this I'll commit.

picked out 2 small nits.

Thanks!