@heddn

Could we add a test

This makes me think we might have an issue if someone has the wrong permissions assigned. Any suggestions how to make this work in a BC manner?

Commented

Thanks for the contribution.

There aren't any deprecation warnings any more from this.

Comment posted.

Is there any way to get a test added for this? What is moderation dashboard that a test block or controller couldn't add so we can increase the scenarios tested for this module?

But the patch needs to be rolled into an MR to ever stand a hope of getting merged. We do that so it has tests run and is reviewable. Leaving at NW for this next task.

Thanks for that explanation.

There s a 2.0 stable release as of today. Is that an option now? Alternatively, we could backport this.

Thanks for the contributions

Can we roll into an MR so tests can trigger?

Thanks for the fixes.

This is not the case any longer.

We should add some doxygen about how to use this new feature. Otherwise, this is pretty much set to go.

Patch needs conversion into an MR. Also, is this an issue on 2.x? If it isn't then we can fix 1.x, but let's confirm it is or isn't there first.

Thanks for the contributions.

heddn → made their first commit to this issue’s fork.

The patch needs to be rolled into an MR for tests to execute.

There's 2 competing MRs and no tests in either. Lets decide on which MR we want to use, close the other and ideally add some tests.

This should have tests.

Drupal 11 release tagged.

https://www.drupal.org/project/layout_builder_st/releases/2.0.0 →

This is now resolved. With passing tests, we can now tag a release.

Thanks for the contributions.

It seems to be some type of race condition. If I add a few sleeps into things, then it starts to collect. But it seems to collect a lot of noise from flood, session and previous user login that wasn't there previously. I'm not sure what the difference between functional and javascript is, but there seems to be something.

diff --git a/core/modules/system/tests/modules/performance_test/src/PerformanceDataCollector.php b/core/modules/system/tests/modules/performance_test/src/PerformanceDataCollector.php
index d026b756d29..92adf9059c5 100644
--- a/core/modules/system/tests/modules/performance_test/src/PerformanceDataCollector.php
+++ b/core/modules/system/tests/modules/performance_test/src/PerformanceDataCollector.php
@@ -81,6 +81,7 @@ public function destruct(): void {
     if (!$lock->acquire('performance_test')) {
       $lock->wait('performance_test', 10);
     }
+    sleep(2);
     $collection = \Drupal::keyValue('performance_test');
     $existing_data = $collection->get('performance_test_data') ?? [
       'database_events' => [],
diff --git a/core/tests/Drupal/Tests/PerformanceTestTrait.php b/core/tests/Drupal/Tests/PerformanceTestTrait.php
index 02872b84f48..04cb3ecce65 100644
--- a/core/tests/Drupal/Tests/PerformanceTestTrait.php
+++ b/core/tests/Drupal/Tests/PerformanceTestTrait.php
@@ -112,6 +112,7 @@ public function collectPerformanceData(callable $callable, ?string $service_name

     $session = $this->getSession();
     $collection->deleteAll();
+    sleep(15);
     $performance_data = new PerformanceData();

Thanks for the contributions.

Drupal 11 is now working, but Drupal 10 is now failing. Getting closer.

I'm confused. I've converted the JSON::API test over but the db collector isn't getting triggered to collect queries.

Any chance we can add a quick test case for this?

Can we roll this into an MR so tests can execute? Ideally we add tests.

I think this is long out of date. If that isn't the case, feel free to re-open and provide an updated direction.

I think this is long out of date. If that isn't the case, feel free to re-open and provide an updated direction.

Can we add tests?

heddn → made their first commit to this issue’s fork.

Marking as duplicate of one or both of the mentioned tickets in #10. If this is inaccurate, feel free to re-open and document what else should be done.

The MR needs a rebase.

The latest changes in the MR are still failing tests.

There's not much we can do here. Except point to a blog post or a wiki page showing how to do this.

The patch should be rolled into an MR. Ideally we add test coverage.

The patch should be rolled into an MR. Ideally we add test coverage.

Those issues should be resolved fairly quickly, so I'm not going to move forward here unless those stall out for some reason. If they do stall, we can reopen and consider doing these changes.

The only worry I have with CI test failures is the cspell on dataparser. If we can add that to the allow list, then this looks good to me. The default previous version of drupal core is about to change any day now so I'm not too worried about getting that green.

Can we covert that patch into an MR so we can see how tests fair? And ideally add a test while working on this?

Thanks for rerolling this.

That makes sense. I guess if we wanted to we could add a test module with the legacy annotation and test for that. But not its fixed, it isn't likely to happen again.

Can we add tests for this scenario?

Can we add test coverage for this?

phpunit test failures

need to add a .cspell-project-words.txt with the names of the module maintainers to avoid cspell errors.

heddn → made their first commit to this issue’s fork.

heddn → made their first commit to this issue’s fork.

Thanks for the contributions here.

Posted some minor feedback on the PR. I did no manual testing.

If this is all about tracking a new release, updating the title.

heddn → made their first commit to this issue’s fork.

There are tests failing now.

heddn → made their first commit to this issue’s fork.

Any chance of this bug fix going back to 11.2 as well?

heddn → created an issue.

I agree, this is a duplicate of https://www.drupal.org/project/layout_builder_st/issues/3497945 📌 Make coding standards fixes Active

https://www.drupal.org/project/layout_builder_st/issues/3497945 📌 Make coding standards fixes Active seems to have resolved this indeed.

Thanks for the improvements here.

heddn → made their first commit to this issue’s fork.

Thanks for your kind fixes. Now, why didn't tests catch that?

is the thought to merge that over here?

Also, merge conflicts still seem to exist.

Posted feedback on the MR.

I assume that once this gets merged, we'll add the link target to https://www.drupal.org/node/3223395#s-migrate-drupal? Ah, just reviewed the IS. It is in there as next steps.

I don't see a CR though. Do we still need to add that? Otherwise, +1 on RTBC.

Needs a rebase.

Can we add some screenshots showing before/after the problem and its fix?

Can we get before/after screenshots showing the problem and fix?

Lets land this. Its very disruptive. But it will always be disruptive. And much of the RTBC queue is cleaned out right now, so this is about as good as it gets.

Thanks for your contributions.

MR 102 looks like an easier solution. But I'm not sure which is the better approach. I don't use json parser all that often myself to have an opinion. Any suggestions? MR 102 doesn't need a rebase/reroll but 103 does (if the later is the preferred solution).

heddn → made their first commit to this issue’s fork.

Reasonable plugin. Needs tests though. And there's some feedback posted on MR.

Thanks for the contributions.

There's no code to review. Back to active.

I'm not sure how the actual fixes in MR 96 are fixing the problem. Is there any way to add a test for this?

NW for adding the final bits.

The approach in MR 111 seems reasonable.

Is there any real world testing for MR 109? What's in there looks reasonable.

I think I'd like to see #3227245: Entity lookup doesn't work with multiligual content → land first to make the work here cleaner and easier to review.

I changed the base branch and attempt to rebase using the gitlab UI. There are still conflicts though. In general, the changes here all make sense. Great work here.

I'll take comment #7 as a good way to keep things a bit more DRY. If you still feel strongly about the addition of this feature, feel free to re-open and propose some alternatives.

can't those values be empty arrays? that's what I'd expect given the return type of getMigrationDependencies and getMigrationTags. They should always return an array. With that in mind, I'm going to close this as won't fix. If you strongly disagree, feel free to re-open and provide some examples of where an empty array might not be possible.

heddn → made their first commit to this issue’s fork.

Thanks for the work here.

Thanks for your contributions.

Thanks for the work on this everyone.

Nice idea here. Added some comments on the MR.

There's no adjustments to tests and the existing MR needs a rebase.

Node body addition got removed in core at some point in the recent-ish past. We'll need to add it to all our config exports.

I like the idea here. Let's roll this into an MR and see how tests like it.

Tests are failing.