OgComplex hardcoded to og:default selection handler

[yanniboi]

Not sure if this is relevant to #17.

Problem/Motivation

When editing a field of field type OgMembershipReferenceItem (like og_group_ref) if you change the selection handler (for example to an entity reference view) the entity reference field widget (OgComplex) does not use the views selection handler, it is hardcoded to use 'og:default'.

Proposed resolution

Not entirely sure how to architect the solution, whether 'og:default' should be able to adapt depending on the selection handler defined in the field settings or whether OgComplex should look up the selection handler and pass over to ViewsSelection rather than OgSelection.

[damiankloip]

IMO this is fine, I think what you may be looking for is that we allow the selection handler that Og uses are the base query to be configured. Which is currently not allowed by design.

EDIT: To clarify, I am not saying it shouldn't be, just that you can't do it right now.

[amitaibu]

In D7 we do allow people to use their own selection handler, and D8 should follow it as-well

With that said, we should keep people from doing mistakes that could cause data corruption (e.g. referencing non-group).

[andrewbelcher]

@amitaibu How hard a requirement is it to ensure that you can't end up with data corruption/access issues? Can some of that be left to site builders if we give them information to make the decision/warnings?

Specifically, OgSelection will only every work with other selection handlers that extend DefaultSelection (as it doesn't do anything until ::buildEntityQuery()). To get support for handlers that don't extend DefaultSelection, we'd need to either:

• Switch to only using things as defined in SelectionInterface: This would likely have a significant performance regression as we'd have to manually process the results to ensure everything is an accessible group.
• Have two selection handlers and use the one that extends DefaultSelection when the selected handler extends DefaultSelection and a more versatile one directly implementing SelectionInterface for other handlers. This means the performance regression is limited to where there is no other option.

Either way, the main bit I'm stuck on at the moment is figuring out exactly what from OgSelection::buildEntityQuery() would need to be done in our alternative selection proxy. I can see it ensure that we only return group entities (am I right in thinking that in D8 all entities of a group-able bundle are group-able?).

The rest of it appears to be about the complex widget (own groups/other)? That we could manually process, though Though I'm confused as to how you can return if $user_groups is empty regardless of whether you are in admin or other field mode?

We could probably support ViewsSelection by having something that integrates more deeply in the same way with do for DefaultSelection. One option for how to solve both of those is to pass them in as part of entity_reference_options. ids is already there... The is a valid group constraint may be harder.

Not trying to make lots of work for you! @yanniboi and I are happy to do the leg work, but want it to be viable and do it correctly...

[andrewbelcher]

I've created a PR (#151) as I thought some code may help pick apart the issue and see whether my solution would be viable. It's not tested yet - will take a look at making it 'functional' tomorrow...

[andrewbelcher]

So that is now 'operational'... It's not finished yet, as OgViewsSelection doesn't apply the group bundle check or the ID exclusions at the moment. In a simple case, these may both be fine to add in, so can carry on from here if we feel this is a viable/the right approach to take.

My main concern is that what should really be passed on is ::getReferenceableEntities() and ::validateReferenceableEntities() - these are the two relevant methods from SelectionInterface that are actually called on the proxy plugin. For the specific cases of DefaultSelection and ViewsSelection this is fine, as we know all the other logic. However, it may not work for anything that overrides either of those methods and puts it's own logic in them. So we may have to be even more explicit with which classes we support. In which case we may need to provide some way for other modules to declare support for them...

As an example, PhpSelection does override ::getReferenceableEntities() to apply some additional logic - it further restricts the results. In that case, the additional restrictions would not be applied as PhpSelection::getReferenceableEntities() never gets called. The only solutions I can think of to that is to switch to applying our conditions in alters (e.g. hook_query_entity_reference_alter()/hook_views_query_alter or copy/paste PhpSelection::getReferenceableEntities() into OgPhpSelection::getReferenceableEntities() and similar for any other plugins...

I'm going to hold off doing any more work on this until you've had a chance to look at it and see whether this is a good approach...

[amitaibu]

How hard a requirement is it to ensure that you can't end up with data corruption/access issues?

What's harder than platinum? ;)

Seriously, I'd like the API level to come with guarantees. A site builder assumes we're not allowing them to screw things up too badly.

Even for an advanced dev, we should allow you to inject your own business logic - but still have those guarantees in place.

The current PR seems a little off. Maybe @damiankloip will have an idea for a better approach, given his Views experience.

[andrewbelcher]

I'd be interested to hear what you think is a little off in the PR. Is it just grokability or the fundamental approach?

At the moment, I'm not sure there is only a guarantee that the entity wont be a group, but it could still be corrupt in any other sense that other handlers specify...There is nothing in terms of a developer being able to add in their own logic, which could include access or other validation, unless they do it in ::buildEntityQuery(). The proxy handler ignores overridden ::getReferenceableEntities() and ::validateReferenceableEntities() - as well as anything from SelectionWithAutocreateInterface implemented in the overriding class.

So unless damiankloip thinks a different approach is better (and the views stuff is a really small part of this that could easily be a follow-up - it seemed an easy win once the handlers were selectable), I would suggest to get closer to harder than platinum support we do the following:

• Make the OgComplex widget disable autocreate.
• Make it so that our handler only supports specific handlers:
  • DefaultSelection: This one is fine.
  • TermSelection: This one changes the formatting of the results in ::getReferenceableEntities() which will be ignored - could potentially live with that as it's UI rather than data integrity..
  • CommentSelection: Overrides autocreate stuff which we'll disable.
  • PhpSelection: Is not supported (though is not used in core - meant for extending I think).
  • NodeSelection: Overrides autocreate stuff which we'll disable.
  • FileSelection: Overrides autocreate stuff which we'll disable.
  • UserSelection: Overrides autocreate stuff which we'll disable.
• Either in this or a follow up, make sure that the views proxy is able to apply both the bundle constraints and exclude IDs (which I think both, certainly the latter can be done using hook_views_query_alter()).
• In a form alter and/or validation, ensure that you can't configure an unsupported handler.

[damiankloip]

I agree with @amitaibu, the PR seems a little on the hacky side. Sorry. It also returns responsibility back to the Og class itself. Creates an instance there to get the id to pass into the form element. I don't think that's ideal.

I still think we could do all of this in the OgDefault class, but we would use the alterEntityQuery() method instead. This is on the SelectionInterface, so it can be relied on.

However... there is currently one issue with that which would need to be fixed first, and that is https://www.drupal.org/node/2666202

[damiankloip]

A hook_views_query_alter would be problematic without the additional tags and metadata on the query as we would not know anything about the selection handler. We would need that to be able to implement this logic.

[andrewbelcher]

@damiankloip thanks for taking a look at the code.

I am unsure as to whether ::alterEntityQuery() will work. Have I misunderstood something?

The implementation of ::alterEntityQuery() that would be called is on the selection handler added as metadata to the query, which happens in ::buildEntityQuery(), so if we're continuing to pass that off to the selected plugin, it would be the selected plugin's ::alterEntityQuery() that is called (e.g. NodeSelect::alterEntityQuery() rather than OgSelection::alterEntityQuery().

Or were you were thinking we could leave it entirely to the selected plugin? In which case, we could instead alter the config form to ensure the selected handler and configuration is valid and query alters (which would also depend on an addition to core so that the views selection passed on the full selection info) and not have an OgSelection at all.

Have I understood you correctly?

[damiankloip]

Hmm, yes. This could pose some issues, you're right. That puts a nice clean solution to bed then :)

Although, maybe we could just implement our own alter on the query (hook_query_entity_reference_alter) but add our own settings into the handler settings when we create the child instance. We would then be able to use that in the alter hook to implement the logic for both types of selection handler maybe?

• disclaimer* have not looked at the code whilst writing this...

[andrewbelcher]

Yes, I was thinking that hook_query_entity_reference_alter() would allow us to do most things. If we can get core ER/views to pass on some more information to the view, we can also do what we'd need for ViewsSelection (or we may be able to do some of it via additional info in handler_settings.

Those two things lead me to wondering if we can drop OgSelection entirely, but would probably also require us altering the config form to make sure that the only selection handlers that are provided are ones that we know we can support (currently ones that extend DefaultSelection - hopefully soon ViewsSelection)... What do you think?

[andrewbelcher]

Based on a quick chat with @damiankloip in IRC, I'm going to try putting together some code that moves away from using OgSelection as a proxy for selection handlers. This will test the viability of this as an alternative approach and allow us to consider it against other approaches.

He mentioned that @amitaibu had previously talked about wanting to use if for other purposes, so I will keep it in place and find a way to prevent duplicating code - we can always remove it later if we feel it's no longer needed.