Make coding standards fixes

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

ok looks good now thanks.

I'm still seeing lots of problems flagged in the phpcs job: https://git.drupalcode.org/issue/layout_builder_st-3497945/-/jobs/3931061

If the goal of this issue is to fix the coding standards issues, that job should be passing. Let's also get this branch synced up with 8.x-1.x.

I have rebased the branch. The things that are left will require manual fixes.

Looks like pretty good progress here! Just made a couple of small suggestions, but keep hacking on it!

Hello @liam morland , this looks great, could I help with fixing some of the phpcs issues that I still see?

Feel free to make coding standards fixes. That is what this issue is for.

I fixed most of the issues (comments issues) but I am looking at the PHPCS issues in the test directory and I see a return statement like this:

 return;
    $this->updateBlockTranslation('.block-system-powered-by-block', 'untranslated label', 'label in translation');

    $assert_session->buttonExists('Save layout');
    $page->pressButton('Save layout');
    $assert_session->addressEquals('it/node/1');
    $assert_session->pageTextContains('label in translation');
    $assert_session->pageTextNotContains('untranslated label');

    // Confirm that untranslated label is still used on default translation.
    $this->drupalGet('node/1');
    $assert_session->pageTextContains('untranslated label');
    $assert_session->pageTextNotContains('label in translation');

    // Update the translations block label.
    $this->drupalGet('it/node/1/layout');
    $this->assertNonTranslationActionsRemoved();
    $this->updateBlockTranslation('.block-system-powered-by-block', 'untranslated label', 'label updated in translation', 'label in translation');

    $assert_session->buttonExists('Save layout');
    $page->pressButton('Save layout');
    $assert_session->addressEquals('it/node/1');
    $assert_session->pageTextContains('label updated in translation');
    $assert_session->pageTextNotContains('label in translation');
  }

Any reason why is that return; statement set up like that? I am assuming the tests below are not passing at all or something.

Sorry to bother on this.

The return might have been added to disable the tests following it. It may be that the tests only pass if some of them are disabled so this was done temporarily until the tests can be fixed.

You can use git blame to find out what commit added that line.

I removed the return statement and commented the tests with a message "TOFIX".

I'd like to look on these later, but I am short of time and some of the issues are quite hard to fix. I fixed the other PHPCS and PHPSTAN issues.

I think it would make sense to commit this, even with some tests commented-out. The tests can be fixed in a follow-up. That way, going forward, other merge requests can be done on the basis of always having tests pass.

ok, I'll look into the issues at #3431600 - Automated Drupal 11 compatibility fixes for layout_builder_st 📌 Automated Drupal 11 compatibility fixes for layout_builder_st Needs review and see if I can help there.

@phenaproxima, this is ready for merge. We merge this in, tests pass, then we can merge these fixes into the Drupal 11 upgrade merge request , it will be less changes for you to review at that point.

Thanks for all the hard work on this @Liam Morland and @danrod.

This is a critical issue for us as the layout_builder_st module is a release blocker for us. We need a tagged release in order to publish our tagged release for our distro (WxT).

This disables most(?) of the test coverage without explanation, which I don't think is acceptable. This means we might well be committing a completely broken module, which is broken for reasons we don't understand. If we don't fix at least most of these problems now, nobody will in the future.

I see one extremely strange thing in the services.yml file, which might explain some of the test failures?

I spoke with @phenaproxima on Slack, preparing layout_builder_st as discussed with him.

Basically we're going to leave the failing tests in, as the related issues need to be fixed, I'll revert the disabling of those tests and we'll create a followup issue to fix those 4 remaining fails.

I've made a new branch, cherry-picked the best cs and required deprecation fix from the other branch. It was easier to do a new branch to keep the scope down. Focusing on a basic level of phpcs fixing and the required deprecation fix. Looking to see the number of fails, expecting 14 or less fails on the current major.

I recommend reviewing https://git.drupalcode.org/project/layout_builder_st/-/merge_requests/26 MR 26. Someone should close the other as we do not want to disable tests unless there's a good reason to do so.

joseph.olstad → changed the visibility of the branch 3497945-make-coding-standards to hidden.

@phenaproxima, thanks for this. There's a lot of changes accumulated and they look good, would be good to merge this in and we can open up a followup issue for the 4 test failures. We will definately be putting the 8.x-1.x branch through it's paces in real world scenarios using D11 once this is merged.

@danrod, @liam morland , I haven't had a chance to test this merge request on Drupal 11 yet, it looks like it'll probably do the job. Test results are similar to what we achieved in the 3431600-automated-drupal-11 branch.

Hoping this gets merged into dev, that would be enough to unblock our Drupal 11 WxT release. Even if it's broken but D11 compatible we would be able to patch fix it without using a composer override.

We have a may 2nd event where we need to release Drupal 11. Without a merge by friday April 25h we are left with no choice but to fork layout_builder_st and publish a Drupal 11 compatible version. We've done all we can here, it is time to move forward!

Alright, let's deal with test failures in a follow-up. Can you open that and link it here, please?

Using 2.0.x , rebuilding cache, I get this error:

TypeError: Drupal\layout_builder_st\DependencyInjection\ClassResolver::__construct(): Argument #1 ($decorated) must be of type Drupal\Core\DependencyInjection\ClassResolverInterface, Drupal\Core\DependencyInjection\Container given, called in /var/www/clients/client1/web1/web/docroot/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in /var/www/clients/client1/web1/web/docroot/html/modules/contrib/layout_builder_st/src/DependencyInjection/ClassResolver.php on line 14 #0 /var/www/clients/client1/web1/web/docroot/html/core/lib/Drupal/Component/DependencyInjection/Container.php(259): Drupal\layout_builder_st\DependencyInjection\ClassResolver->__construct()
#1 /var/www/clients/client1/web1/web/docroot/html/core/lib/Drupal/Component/DependencyInjection/Container.php(177): Drupal\Component\DependencyInjection\Container->createService()
#2 /var/www/clients/client1/web1/web/docroot/html/core/lib/Drupal/Component/DependencyInjection/Container.php(430): Drupal\Component\DependencyInjection\Container->get()
#3 /var/www/clients/client1/web1/web/docroot/html/core/lib/Drupal/Component/DependencyInjection/Container.php(237): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters()

I'll create a merge request for the purposes of our release

How do I reproduce #35 on a clean Drupal 11 install with Layout Builder ST 2.0.x? I'm not seeing it. I don't think this will happen on new installs, only existing ones that go directly from 8.x-1.x to 2.0.x, because the service definition changed.

An empty post-update function should cause a container rebuild, so we should probably just add one -- in a new issue -- to deal with that.

yes this was testing a drop-in replacement for our patched version based on layout_builder_st 8.x-1.x , I ran cache rebuild several times with drush and unable to shake it. Is there a drush command for doing a container rebuild?

I'm seeing this as a possibility:

drush ev '\Drupal::service("kernel")->invalidateContainer();'

Normally drush cr will do the trick.

Ya I ran drush cr several times, also restarted php8.3-fpm , didn't help.

Well, that's annoying. In that case, I guess I'd try going into the database directly and truncating all of the cache_* tables.

ya, I'll try that, however it is tough to expect all our clients to do that.

That's why I'm not suggesting that. Quoting #38:

An empty post-update function should cause a container rebuild, so we should probably just add one -- in a new issue -- to deal with that.

I'm just waiting for you to confirm that clearing the cache tables (so as to force a container rebuild) does the trick. :)

@phenaproxima

That doesn't work either. I truncated all the cache_ tables. like this:

TRUNCATE cache_access_policy;
TRUNCATE cache_bootstrap;
TRUNCATE cache_config;
TRUNCATE cache_container;
TRUNCATE cache_data;
TRUNCATE cache_default;
TRUNCATE cache_discovery;
TRUNCATE cache_discovery_migration;
TRUNCATE cache_dynamic_page_cache;
TRUNCATE cache_entity;
TRUNCATE cache_font_awesome;
TRUNCATE cache_menu;
TRUNCATE cache_migrate;
TRUNCATE cache_render;
TRUNCATE cache_toolbar;
TRUNCATE cache_tweets;

╭─◀ ☕ ryzen ▶ /docroot ▶ 📂12 📃15 🔗4 ▶ 🔀 11.1.x ▶
╰❯ $ drush cr;

In FieldStorageConfigStorage.php line 167:
                                                                                                                                             
  Unable to determine class for field type 'layout_translation' found in the 'field.storage.node.layout_builder__translation' configuration  
                                                                                                                                             

In DiscoveryTrait.php line 53:
                                                                                                                                                                                       
  The "layout_translation" plugin does not exist. Valid plugin IDs for Drupal\Core\Field\FieldTypePluginManager are: comment, context, datetime, entity_reference_revisions, file, fi  
  le_uri, fontawesome_icon, image, layout_section, link, metatag_computed, metatag, list_float, list_string, list_integer, path, text_long, text, text_with_summary, webform, uuid, c  
  hanged, decimal, password, uri, language, entity_reference, timestamp, created, email, map, string, string_long, integer, float, boolean

It got us past the container problem, though, didn't it! So that's progress.

Next step is to open an issue to add an empty post-update hook to force a container rebuild. Let's deal with this new problem separately, in yet another issue. I really don't want to just keep throwing problems at one mega-issue. Small, tightly scoped issues we can quickly iterate on, that's the ticket.

https://www.drupal.org/project/layout_builder_st/issues/3520833 📌 add an empty post-update hook to force a container rebuild Active

🐛 The "layout_translation" plugin does not exist. Active

Thank you!!

There are still a few phpcs issues on branch 2.0.x. They are all "Missing short description in doc comment". Perhaps this issue should stay open for those to get fixed.

@liam morland , let's open a new issue for those

Yeah, let's deal with those later. A lot of those are just because there's not much point in committing useless doc comments comments like "This class implements SomeInterface" (since you can see that by reading the code). It's better to leave a few coding standards violations in place and fix them usefully later in a separate issue, when we still were able to fix a lot of other coding standards issues in this one. 👍