Feature: netcall integration

[Adnan-cds]

Templates for:

• Header
• Footer
• Empty main content

Test URLs

• Header template
• Footer template
• Template with empty body
• Existing ModernGov template

Resolves #630

[Adnan-cds]

Test failure for Drupal 10.x is unrelated. So putting forward for review.

[finnlewis]

This looks good to me, but I'd like a second opinion.

@ekes @stephen-cox either of you able to give this a quick review from a more back-end perspective to check there's no security or performance concerns?

[finnlewis]

We do really want a developer to code review this for security / performance before merging.

@ekes points out it is quite an involved to review properly.

• it's only running on one route
• just want to check that it's not doing anything funky with the dom.

@Adnan-cds thinks this is quite urgent.

We've just talked through the code and how it works, which should help anyone who's able to find some time to dive in deeper from a back-end perspective.

@ekes will hopefully find some time this week.

@willguv we note that this is a priority from your perspective, please bear with us!

[willguv]

No worries @finnlewis thanks for spending time on this

[ekes]

Stepping through this still make me feel like there's something there that could be abused, and that there must be some other way. But I neither can find it. Nor without working on the whole Symfony response handling can I think of a easier way.

So that said. The one thing I did notice is this is handling all requests and so breaks Big Pipe. It's this sort of thing where something else is in the response too that is hard to take account of.

However in that specific case we could just disable big pipe:

diff --git a/localgov_moderngov.routing.yml b/localgov_moderngov.routing.yml
index 3f8f6bc..f0dbaaf 100644
--- a/localgov_moderngov.routing.yml
+++ b/localgov_moderngov.routing.yml
@@ -7,5 +7,7 @@ localgov_moderngov.modern_gov:
     assoc: 1
     depth: 2
     options: 3
+  options:
+    _no_big_pipe: TRUE
   requirements:
     _permission: 'access content'

[Adnan-cds]

Hi Ekes, Sorry, taking a while to respond.

Anything dependent on Javascript including BigPipe functionality is mostly broken on the individual Header and Footer templates as not all Javascript is available as part of these templates. But once these two templates are combined in a single page, things should work. Could you please confirm if BigPipe is broken in the Template with empty body which carries all the scripts. If yes, we will definitely have to turn BigPipe off. Otherwise we can put some more thoughts into it.

[Adnan-cds]

And thanks for looking into it :)

[ekes]

Hi Ekes, Sorry, taking a while to respond.

Anything dependent on Javascript including BigPipe functionality is mostly broken on the individual Header and Footer templates as not all Javascript is available as part of these templates. But once these two templates are combined in a single page, things should work. Could you please confirm if BigPipe is broken in the Template with empty body which carries all the scripts. If yes, we will definitely have to turn BigPipe off. Otherwise we can put some more thoughts into it.

At least with the site I was testing with I was getting a full fatal error when Big Pipe was processing some part that was NULL.

[Adnan-cds]

Sorry, still not clear. Which template led to the fatal error? All four templates or a particular one?

[ekes]

All four templates or a particular one?

Just requesting the header and footer ones threw the fatal error.

[Adnan-cds]

Okay, thanks for confirming. I will see what I can do. I would prefer to keep BigPipe on. But if there's no other way then we will just have to turn it off as you have suggested.

[Adnan-cds]

I couldn't reproduce the fatal errors, but I did see PHP warnings like Warning: Undefined array key 2 in Drupal\big_pipe\Render\BigPipe->sendPreBody() (line 333 of web/core/modules/big_pipe/src/Render/BigPipe.php). At the same time realized BigPipe is mostly relevant for logged in users whereas the templates from this module are usually going to be consumed by anonymous users. This means BigPipe isn't very relevant here. So I have turned it off just as you have suggested.

[finnlewis]

Discussing in Merge Tuesday, @ekes has reviewed, @Adnan-cds has made changes and we're happy to merge and release this.

[Adnan-cds]

Thanks.