Attachments and Cacheability dropped for Leaflet (embeded|ajax) popups

I can confirm this from the source code: Quite some code did not make the D8 transition with BubbleableMetadate, containing Attachments (CSS/JS) and Cacheability. Raising prio as cacheability bugs lead to information disclosure.

Adding followup.

Patch flying in that should fix these codepaths:
* 1) The field formatter
* 2) The "rendered entity" popups in the LeafletMap views style
* 3) The "rendered entity ajax" popups in the LeafletMap views style partially: i could fix the cacheability, but to fix attachments (js/css) we need #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS so created followup #3069250: Leverage ajax attachment commands in leaflet popups to address it there.

I manually tested that for 3) the cacheability was broken without (changed nodes did not update) but works with the patch.

I tested that for 2) the Blazy js at least works better (without: no image, with patch: throbber is shown).

My gut feeling is that this is partly a blazy issue, so partly discussing in #2969221: Make blazy work in Leaflet (embedded|ajax) popups.

(I'm using blazy as example and assume if we make it work "the right way", we'll make any JS work.

Hmm, as of the blazy part: I wonder: Does leaflet fire "attach drupal behaviors" when it opens a popup (it looks like it copies some html from drupalSettings to the dom).

EDIT: No it does not and that's the problem: #3069259: Fire Drupal.attachBehaviors when embedded popup opens

Thanks all ... folks. I am following this, but sincerely hardly getting all the picture. Also because this issue is branching into new ones ... that seem all related each other (but independent at the same time ... ?, otherwise why new issues for each of them?).

I don't have great time to test this all now. But of course I would trust Drupal seniority higher than mine.
Please go on, on all this/these, so that it might be tested and reviewed (better as a whole patch).

Once RTBC will be happy to commit all in dev and next release ...

OK luckily i could fold all sattelite issues back into this patch, as core improves but does not change its API.

Cumulated patch flying in.

I'm quite confident as of the views popups, but really like to get some worksforme for the field formatter.

thanks to both of you ...
@anruether you really seem to be the best reviewer and real use cases tester on all this.
It is crucial to enhance this issue use cases BUT without injecting regressions on existing functionalities.
This module D8 version is being used widely and worldwide ...

#10:
> I'm quite confident as of the views popups, but really like to get some worksforme for the field formatter.

Yeah thanks for pointing out that obvious regression in the field formatter. Hackedyhack, working on this.

Yeah, just omitted a parameter in the token->replace() function signature leading to #3069364: WSOD when Popup Infowindow enabled and token module installed. Fixed, FieldFormatter needs manual test.

thanks @axel.rutz ... super reactive and efficient contributions here.
I am confident that @anruether will further review ...
and I will do too, as soon I find a couple of spare hours.

Cool work @axel.rutz ...
I tested this patch on my Leaflet dev module playground / demo app, and it really seems to work well and solve the Blazy integration into the Leaflet popups.
Great!

The only minor issue that I experience is that, with this patch applied (an d not without), once (and almost every time) I enter a Leaflet Map View settings page or a Manage Display page containing a Leaflet Map Formatted field, the application throws and prints the following error/warning:

Notice: Array to string conversion in Drupal\Core\Asset\AttachedAssets->setLibraries() (line 53 of core/lib/Drupal/Core/Asset/AttachedAssets.php).
Drupal\Core\Asset\AttachedAssets->setLibraries(Array) (Line: 41)
Drupal\Core\Asset\AttachedAssets::createFromRenderArray(Array) (Line: 155) ... 

...
Notice: Array to string conversion in Drupal\Core\Asset\AttachedAssets->setLibraries() (line 53 of core/lib/Drupal/Core/Asset/AttachedAssets.php).
Drupal\Core\Asset\AttachedAssets->setLibraries(Array) (Line: 41)
Drupal\Core\Asset\AttachedAssets::createFromRenderArray(Array) (Line: 155)
Drupal\Core\Render\HtmlResponseAttachmentsProcessor->processAttachments(Object) (Line: 45) ... 

...
Notice: Array to string conversion in system_js_settings_alter() (line 773 of core/modules/system/system.module).
system_js_settings_alter(Array, Object, NULL) (Line: 539)
Drupal\Core\Extension\ModuleHandler->alter('js_settings', Array, Object) (Line: 328)
Drupal\Core\Asset\AssetResolver->getJsAssets(Object, ) (Line: 322) ... 

It seems really related to the #15 patch.

Hard for me to find out (and fix) the cause ... I attach the .txt file with all the error/warning txt message

Thanks to a PM from @anruether i could fix an edge case where $render_context was empty. Still need to address #18.

OK now to #18:
> [notices] once (and almost every time) I enter a Leaflet Map View settings page or a Manage Display page containing a Leaflet Map Formatted field

Thanks! So is it a map view or a view containing a formatted field?

I tried to provoke this like you wrote in our installation but could not succeed. Something must be different here vs there.

The warnings are quite clear: Render arrays pass a list of libraries in $somebuild['#attached']['library'], which is an array of strings.
In the error you found, some element in this array is not a string as expected, but itself an array.
I read every line of the patch and can't spot a possible violation of this contract that is introduced there (but it may be that the patch uncovers a violation elsewhere).

To spot this, i'd simply add a dpm($results['#attached']['library']); before the final return $results; in LeafletDefaultFormatter::viewElements. One of the elements probably is not a string. We can then see who violates the contract.
(If you pass me sufficient sourcecode rights or a DB dump i can look into this.)

May or may not be: Searching for "Warning: explode() expects parameter 2 to be string, array given in Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies()" leads to:

For those who find this via googling the error message:
I got this error because of incorrect indentation within my theme's themename.libraries.yml file

Relating to this warning:

Notice: Array to string conversion in Drupal\Core\Asset\AttachedAssets->setLibraries() (line 53 of core/lib/Drupal/Core/Asset/AttachedAssets.php).

... with a proper xdebug breakpoint I caught that not all the $libraries elements are "strings", but a drupal/ajax array is also injected (@see attached screenshot)
I feel this is coming from this issue patch.

and even a more clear screenshot that shows the content of that array ("drupal/ajax" indexed array of two "drupal/ajax" identical terms ... weird and suspect, isn't it?)

Thanks!! That tells me what i need. Hacking.

Documented this: https://www.drupal.org/docs/8/creating-custom-modules/adding-stylesheets...

Lesson learnt and documented. Fixed patch flying in.

(The difference was: Field cardinality.)

Cooooooooooooooooooool!
@axel.rutz .... you mastered all this, and this planet should be thankful to you once more, today.

The #27 patch seems to work nice, with no warnings and errors in my leaflet module dev playground.
I mark this as RTBC, and it really seems ready for commit into dev.

Did you reviews and tests work like a charm too?

Does this solves also the Leaflet Popups & Blazy integration issue, right?
To you the honour to announce this in that issue eventually, @axel.rutz
Please confirm all this, and that this might be part of the next Leaflet 8.x-1.16,
so I would do ...

Yes i am quite confident that code now leverages the D8 APIs nicely and we weeded out all regressions.

IF still something comes up, feel free to PM me.

Cool we mastered this together. It's a pleasure to get instant and exact feedback, and work with both of you so fluffily!

Committed into dev, is being deployed as part of the new 8.x-1.16 release ...

Arggh ... @
Maybe we sang victory too soon.

I am experiencing this bug/error. The Blazy Image correctly appears on the first marker click and popup on,
but not from the second one click on (on the same marker). The Blazy trobber intstead keeps working, and the following js console error generally appears (leaflet.drupal.js:33):

TypeError: $(element).once is not a function. (In '$(element).once('leaflet-popup-behaviors-attached')', '$(element).once' is undefined)

It really seems something still need to be better tuned and coded. Isn't it?

Screenshot attached ...

Good find! We need our JS to depend on core/jquery.once.

mmmhhh ... the js console error seems gone, but still the image doesn't come out on the second (consequent) click.
Don't you experience the same?

Indeed this need to be fixed somehow ...

In your vanilla test instance (http://dev.drupal8.mbm.wod.by/map) the console still throws the error:

TypeError: $(element).once is not a function. (In '$(element).once('leaflet-popup-behaviors-attached')', '$(element).once' is undefined)

so I worry if you applied the #37 patch on that.

But the matter is that I am experiencing the never-ending Blazy trobber also with that #37 patch applied (and without that error) since the second shot on the open PopUp click ... (@new screenshot attached)

... and then are you able to correctly show-up and re show-up (at least twice) the same Blazy Image in the Popup Infowindow?

@axel.rutz .... heeelp! we need to fix this ...

Con't panic ;-). I'll look into this when that dev site sill be up again.

FTR, i already have a gut feeling what's going on...
* when i ported the ajax code from that other module, i did not really grok it
* when working on this patch, i assumed that the module ajaxes only once on the first load, so we have to attach behaviors exactly once
* it looks like this is not the case and the module ajaxes every time the popup opens
* the ideal solution imho is to make the code ajax and attach only once
* if and only if that does not work out, we'll remove the .once from the attach code and do the improvement in a followup "someday/maybe"

Ok, getting inspiration from your #48 clues, I found a working solution, here packed in the attached patch.
Actually getting rid of $.once makes the trick ...
Going to commit into dev and new 8.x-1.17 release ...

Happy this looks stable now!

@anruether that commit removes that line because I discovered that it was the cause of the break of the leaflet views ajax popup for anonymous users.
I tested and reviewed that everything is working nice on my use cases (the Blazy too) without that attachment of the 'core/drupal.ajax' library ...
so I removed it.