Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

The $context_definition->addConstraint('Bundle', [$bundle]); call in FieldBlockDeriver was having no effect.

tim.plunkett’s picture

Status: Active » Needs review
mtodor’s picture

I have applied this patch and the problem still persists for me.

Here are steps how to reproduce:

  1. Clean Drupal install (8.6.x branch)
  2. Apply patch and enable layout_builder
  3. Create new content type Landing Page
  4. After it's created, delete body field from it
  5. Go to view modes and enable Allow each content item to have its layout customized. => Save
  6. Create one Landing Page entity (you just have to provide title, since body field is removed)
  7. And for that new created landing page entity => goto Layout tab
  8. click + Add Block
  9. => there should be multiple field blocks provided
mtodor’s picture

I have quickly added tests if that can help.

Status: Needs review » Needs work

The last submitted patch, 5: 2986033_5.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.34 KB
4.8 KB

Thanks @mtodor!

If I clear the caches and then immediately refresh /layout_builder/choose/block/overrides/node.1/0/content, I see the correct list.
On subsequent refreshes, it breaks again.
Which means we're losing the constraints.

Cleaned up the test a bit.

Status: Needs review » Needs work

The last submitted patch, 7: 2986033-ecd-constraints-7.patch, failed testing. View results

tim.plunkett’s picture

Ugh, that passed for me locally :(

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mtodor’s picture

@tim.plunkett Test works for me locally too .. :)

Btw. I have checked code from #7 and it works fine.

Additionally, I have checked fail for #5 and it fails in the wrong place, so TestBot doesn't like us.
Here is first try to get in the better relationship with TestBot.

P.S. I have selected 8.6.x for testing because I developed on it.

mtodor’s picture

Status: Needs work » Needs review

The last submitted patch, 11: 2986033_11_TEST_ONLY.patch, failed testing. View results

tim.plunkett’s picture

Oh! Misunderstood the fail. That makes sense, since the testbot installs in a subdirectory, and the URL will be different.

phenaproxima’s picture

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ItemLayoutFieldBlockTest.php
@@ -0,0 +1,74 @@
+    // Validate that only field blocks for layouted bundle are present.
+    $validLinks = $page->findAll('xpath', '//a[contains(@href, "field_block%3Anode%3Abundle_with_layout_overrides%3Abody")]');
+    $this->assertEquals(1, count($validLinks));
+    $invalidLinks = $page->findAll('xpath', '//a[contains(@href, "field_block%3Anode%3Afiller_bundle%3Abody")]');
+    $this->assertEquals(0, count($invalidLinks));

Extremely minor nits: $validLinks should be $valid_links (same with $invalidLinks), and we can use assertCount() rather than assertEquals() to validate the counts.

Additionally, I think that CSS selectors are easier to read than XPath queries, so I think we could use a[href*="field_block%3Anode%3A%bundle_with_layout_overrides%3Abody"] instead of the XPath.

Otherwise, I think this is good to go.

mtodor’s picture

Here is the patch that addresses review from #15.

Well, for me XPath is more readable (because I have worked with it a lot in previous jobs) and additionally, our tests are converting CSS to XPath - so I need to take a look at Symfony documentation - https://github.com/symfony/css-selector/blob/v3.4.12/Tests/XPath/TranslatorTest.php :D

I have changed a bit suggested selector from a[href*="..."] to a[href$="..."], it's a bit more strict.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for changing that, @mtodor :) I appreciate it. RTBC once green on all backends.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2986033_16_FAIL.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
tim.plunkett’s picture

Title: The BC layer for EntityContextDefinition in ContextDefinition is incomplete » [regression] The BC layer for EntityContextDefinition in ContextDefinition is incomplete
Priority: Normal » Major

This should go into 8.6.x to avoid a regression.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 52aa9a23d9 to 8.7.x and a8d7a55139 to 8.6.x. Thanks!

   * @see ::__construct()
   * @see ::__sleep()
   * @see ::__wakeup()
   * @see ::getConstraintObjects()
   * @see ::getSampleValues()
   * @see ::initializeEntityContextDefinition()
   * @see https://www.drupal.org/node/2932462
   * @see https://www.drupal.org/node/2976400

Can we get a follow-up to fix this documentation - having an @see that points to the issue that added the code is weird - that's what git blame is for. Also just listed where it is used in @see is bound to get out-of-date as it now is. And these @see :: don't actually work in PHPStorm and so I guess they don't work on api.drupal.org - so they are pretty pointless.

  • alexpott committed 52aa9a2 on 8.7.x
    Issue #2986033 by mtodor, tim.plunkett, phenaproxima: [regression] The...

  • alexpott committed a8d7a55 on 8.6.x
    Issue #2986033 by mtodor, tim.plunkett, phenaproxima: [regression] The...

Status: Fixed » Closed (fixed)

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