Flag::types and Flag::entity_type should be protected

+++ b/src/FlagInterface.php
@@ -56,6 +56,14 @@ interface FlagInterface extends ConfigEntityInterface, EntityWithPluginCollectio
+   * @return array|null

Shouldn't this always be returning an array, possibly empty? It's certainly better DX to not have to is_array() what you get back from this.

I am retesting, locally it works for me...

This makes sense to me.

As for the "Do we return an empty array or null" thing

That opens up a schism between developers that is never going to be settled.

Me: I put the tea in before the milk ... any thing else is just crazy :)

Pragmatically this should go in as locking down the interface is the more important concern.

1. +++ b/flag.module
   @@ -376,7 +376,7 @@ function flag_flag_access_multiple($flag, $entity_ids, $account) {
          ->condition('nid', array_keys($entity_ids), 'IN')
   -      ->condition('type', $flag->types, 'IN')
   +      ->condition('type', $flag->getFlaggableBundles(), 'IN')
          ->execute();
   

   No worse than before, but this will fail if no bundles are checked, it will generate and invalid query or throw an exception, I'm not quite sure what.

   Then again, I think I've seen a issue somewhere that multiple access check is currently dead code anyway..

   With a method, what you *could* do is check for empty and then return all bundles of that entity type...

2. +++ b/src/Form/FlagFormBase.php
   @@ -138,7 +138,7 @@ abstract class FlagFormBase extends EntityForm {
          '#options' => $entity_bundles,
   -      '#default_value' => $flag->types,
   +      '#default_value' => $flag->getFlaggableBundles(),
   

   Then you somehow need to check for that here, however, e.g. with a separate method that returns true/false.

As you say no worse than before....

My proposal, for now, is a sticking plaster - I have just copied the warning that the Flag:type annotation provided into the FlagInterface return type where in the new scheme of things I would expect to see it.

   /**
    * Return the bundles of the flaggable that may be flagged.
    *
    * @return array
    *  An array containing the flaggable bundles.
+ *  A empty array to indicate all types apply.
    */
   public function getFlaggableBundles(); 

Step 2 file another issue to add the new FlagInterface methods to cover the concerns that you raise.

N-people will have N different ways of dealing with this ..and naming things is hard so I will start the ball rolling...

 /**
   *
   * @reutrn bool
   * TRUE If s this flag applies to all types.
   * FALSE if the flag applied to specific set of bundles.
   *
   * @see FlagInterface::getFlaggableBundles()
   */
  FlagInterface::appliesToAllTypes() 

I think this could be used to fix all cases.

1. +++ b/flag.module
   @@ -127,9 +127,14 @@ function flag_entity_extra_field_info() {
   +    $flaggable_bundles = $flag->getFlaggableBundles();
   +    if (empty($flaggable_bundles)) {
   +      $flaggable_bundles = entity_get_bundles($flag->getFlaggableEntityType());
   +    }
   
   +++ b/src/Entity/Flag.php
   @@ -281,6 +281,13 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {
   +    return $this->types;
   

   Follow-up 1: consider ways to improve the DX for the 'applies to all' case. My preference would to hide the implementation, so the getFlaggableBundles() method internally handles giving you all the bundles if the stored value is empty. I do have a sneaking suspicion though that this could cause problems in some cases...

2. +++ b/src/Entity/Flag.php
   @@ -96,7 +96,7 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {
   -  public $types = [];
   +  protected $types = [];
   

   Follow-up 2: let's change this confusing 'types' to 'bundles' or something like that. I thought I'd filed an issue ages ago about renaming various properties of the Flag entity, but I can only find #2457059: Rationalize fields fid and type on flagging entity .

Patch looks good to me: +1 to #14 and I'll commit if we're all happy with that.

  /**
   * Return the bundles of the flaggable that may be flagged.
   *
   * @return array
   *  An array containing the flaggable bundles.
   *  A empty array to indicate all types apply.
   */
  public function getFlaggableBundles();

On second thoughts, I think this needs to be done differently here, rather than in a follow-up. If this method were called getTypes(), then I would expect it to return the $flag->types property verbatim. However, getFlaggableBundles() I would expect to literally all the bundles that apply -- and do the legwork of converting an empty $types array into an array of all the entity type's bundles.

Working on this now.

> However, getFlaggableBundles() I would expect to literally all the bundles that apply -- and do the legwork of converting an empty $types array into an array of all the entity type's bundles

Duh, except as this patch stands, #2409835: [regression] subtypes option should not be required: leaving empty means it appliest to all isn't in yet. So that's immaterial, and adding that second method belongs in the other issue.

Updated patch.

The difference between this and #14 is just a change in the method name getFlaggableEntityType() to getFlaggableEntityTypeId().

Oops. Our service tests were still accessing $types directly.

Wow. Everything's failing.

simplexml_import_dom(): Invalid Nodetype to importsimplexml_import_dom(Object) Drupal\simpletest\WebTestBase->parse() Drupal\simpletest\WebTestBase->drupalPostForm('admin/structure/flags/add', Array, 'Continue') Drupal\flag\Tests\FlagConfirmFormTest->doCreateFlag() Drupal\flag\Tests\FlagConfirmFormTest->testCreateConfirmFlag() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('3', 'Drupal\flag\Tests\FlagConfirmFormTest') 

Has something changed in core in the last 12 hours maybe?

Huh now it's passing. And the test result page doesn't say what timezone it's showing times in, so I don't know when that happened.

I have noticed lots of seemingly random test fails while working on other flag issues over say a period of about 2 weeks....
Lots has changed in Drupal core over that time ... and in particular the last few days as the sprints from DrrupalCon LA wind down.

Is you suspicion that is a design instability with flag, or maybe a test infrastructure thing?

I think it was a test infrastructure thing. The test report said it was failing, EVERYWHERE, all tests, with a message that seemed to be to do with the test's internal browser. And when I checked again an hour or so later, it was all green...

I'll maybe send it for a retest now, to be sure.

Committed. Thanks everyone!