[5.x] Ensure attached only once

This could be achieved with such approach.

  public function attach(array &$element) {
    ..
    $element['#attached']['library'][] = 'photoswipe/photoswipe.init';

    if ($this->isAttached()) {
      // Add photoswipe js settings.
      $options = $this->config->get('options');
      // Allow other modules to alter / extend the options to pass to photoswipe
      // JavaScript.
      $this->moduleHandler->alter('photoswipe_js_options', $options);
      $this->themeManager->alter('photoswipe_js_options', $options);
      $element['#attached']['drupalSettings']['photoswipe']['options'] = $options;

      $this->attached = TRUE;
    }
  }

Which such code, we would attach drupalSettings once. And it does not matter where we set drupalSettings, they will be attached anyway.

We cannot attach it "once" because there might be cases when the element to which it was attached was not rendered. e.g. #access = false and all subsequent elements to which it should be attached will not contain a library at all.

So the solution is only one in that case. We can cache result of photoswipe_js_options hook since it does not contain the actual element so users cannot detect which element is used now, so they always return the same result theoretically. It means that we can cache it without worries. By cache, I mean static variable once per request. I don't see reasons why we need to use actual cache

Also if there are override options we should not cache it and trigger our hooks.

That's the only thing that we can do here.

Thanks for your investigations @dinazaur! Or should we simply close it and leave things as-is? What would you suggest?

Eh, our developer almost finished this implementation, don't want to throw its work away. But I also don't like this thing. I guess we can at least remove all stuff related to this issue, like this->isAttached etc. I will tell him to remove it.

Hi @dinazaur @Anybody
I returned the implementation of this method to the current implementation as in the 5.x branch, but removed the isAttached() method as useless

Looks good to me.

Just for reference here is logic from #16 https://git.drupalcode.org/project/photoswipe/-/merge_requests/119/diffs...

Thanks, let's remove that old disabled stuff.

Automatically closed - issue fixed for 2 weeks with no activity.