Problem/Motivation

BlockDisplayVariant has logic for handling selection criteria, but this is the domain of page_manager's page_variant entity, not variant display plugins.

Proposed resolution

Delete the properties and defaults and update tests where applicable.

Remaining tasks

TODO

User interface changes

TODO

API changes

TODO

Data model changes

TODO

CommentFileSizeAuthor
#5 2646286-ctools-5.patch11.99 KBtim.plunkett
#5 interdiff.txt6.68 KBtim.plunkett
#2 2646286-2.patch6.34 KBeclipsegc

Comments

EclipseGc created an issue. See original summary.

eclipsegc’s picture

eclipsegc commented
Status: Active » Needs review
StatusFileSize
new 2646286-2.patch6.34 KB

Ok, this removes some tests since our access method is now core's which already has test coverage, and generally cleans up stuff that was using this interface.

CTools, Page Manager and Layout Plugin tests are all passing with the exception of an error in layout_plugin for an unrelated ctools change.

Eclipse

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Walked through all these changes with EclipseGC and witnessed the tests passing. :-)

I'm +1 on this, but I'll leave it to @tim.plunkett to RTBC!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/src/Plugin/ConditionVariantInterface.php
@@ -11,6 +11,10 @@ use Drupal\Core\Display\VariantInterface;
+ * @deprecated
+ *   This interface was intended for use with VariantInterface::access but that
+ *   method should not exists in core so neither should this interface.
  */
 interface ConditionVariantInterface extends VariantInterface {

You need to say what version it will be removed in.

But that said, why not just remove it instead of waiting? We're in alpha after all...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt6.68 KB
new 2646286-ctools-5.patch11.99 KB

And honestly, you can just remove ConditionVariantTrait outright too. Sure, page_manager is using it, but wrongly. And I can easily fix it (page_manager will be broken after the CTools release anyway)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Removing the trait sounds great to me! RTBC +1 :-)

eclipsegc’s picture

eclipsegc commented
Status: Reviewed & tested by the community » Fixed

David and I discussed tim's additional removals here and decided while ctools would ideally provide a trait and interface for this sort of behavior, theres are named wrong at this point, and the trait is inconsistent in how to does various things, so perhaps we'll try again later with something that's not variant specific.

Eclipse

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.