Issue-3445440: making localgov guides blocks translatable

[antonio93rs]

This will fix https://www.drupal.org/project/localgov/issues/3445440
and #154

[ekes]

Is this a dupe of #145 ?

If so if you could review that we can get it merged. If not maybe the two should be combined to add what ever go missed in the first?

[antonio93rs]

Is this a dupe of #145 ?

If so if you could review that we can get it merged. If not maybe the two should be combined to add what ever go missed in the first?

Hello @ekes,
This seems to be a dupe of #145 like you mentioned. Sorry, I didn't see it before.
I have been comparing both PR's and mine looks simpler and more efficient due to the next reasons:

• On adds fix to respect translation for header and nav blocks #145 the construct from src/EventSubscriber/PageHeaderSubscriber.php is empty and the entityRepository variable is not being defined and set.
• On src/Plugin/Block/GuidesAbstractBaseBlock.php the entityRepository variable is not being defined and set either.
• It wouldn't be needed to update src/Plugin/Block/GuidesContentsBlock.php if we get the translated version of referenced guides pages on the GuidesAbstractBaseBlock class like I'm doing on this PR. I think it's better to fix the abstract class so that any block extending it would be fixed already.

I hope all of that makes sense.
Cheers.

[ekes]

On #145 the construct from src/EventSubscriber/PageHeaderSubscriber.php is empty and the entityRepository variable is not being defined and set.

It's being set in the constructor declaration by property promotion

public function __construct(protected EntityRepositoryInterface $entityRepository) {
}

I think we generally prefer this method now we no longer need to support PHP < 8.0

This is also the case in GuidesAbstractBaseBlock.

It wouldn't be needed to update src/Plugin/Block/GuidesContentsBlock.php if we get the translated version of referenced guides pages on the GuidesAbstractBaseBlock class like I'm doing on this PR. I think it's better to fix the abstract class so that any block extending it would be fixed already.

This is much better, it also means the cache tags will be correct, as this method is called in the base to set them too

localgov_guides/src/Plugin/Block/GuidesAbstractBaseBlock.php

Line 152 in 7e106dc

$this->setPages();

I've already made a branch #156 to contain the additional 8.2 testing that was in #145
I propose to make another branch that:

• pulls that updated matrix,
• your branch with the neater loading in the base setPages
• and then add a final additional one for the property promotion, and the correct use of NodeInterface rather than Node that was tucked away in getSubscribedEvents in adds fix to respect translation for header and nav blocks #145.

[ekes]

Posted #157

Reviews welcome. If we're quick we can get it merged at Merge Tuesday in 45 mins https://lu.ma/frbxnce1 (It's every week at the same time).

[antonio93rs]

Posted #157

Reviews welcome. If we're quick we can get it merged at Merge Tuesday in 45 mins https://lu.ma/frbxnce1 (It's every week at the same time).

Hello @ekes ,
I reviewed / approved #157. I think ths PR can be closed now that everything has been covered there.

[ekes]

👍