Re-investigate broken YouTube embed links

robbieaverill commented 6 years ago

Previously related issues: https://github.com/silverstripe/silverstripe-cms/issues/2168

We added oembed dispatcher config to cwp-recipe-cms (oembed.yml) which adds the CWP proxy configuration to the core dispatcher class when it is loaded by injector.

Not all of the necessary changes in core made it into CWP 2.1.0, but they should all be there in CWP 2.1.1 - we're having reports of this not working on CWP 2.1.1 now though so it needs reinvestigating.

cc @emteknetnz

NightJar commented 6 years ago

Having just had a discussion with @emteknetnz about this, I'll summarise here: The discussion revolved mostly around why this is a part of a recipe and what would happen if someone decided not to use a recipe.

Module configuration is global defaults, and not specific to platform. It is always set and is easier to modify (if someone knows about it) rather than unset. However in SS4 it at least could be unset.
Recipes are how project level configuration is added, and probably why it is targeted/merged here. Upon installation, if the key is missing from the composer.json project-files (or public-files for that matter) it will be copied from the recipe into the project - where it is then in the user's (developer's) domain, and they can do as they please with it.
This could be a part of a platform specific module (I'm unsure why it's not in cwp/cwp-core), or at least in cwp/cwp-recipe-core, as this would also cover the headless use of framework only.
We can never cover how everyone will use something unexpectedly, but we can provide support as a trade-off. I think maximum value is added this way (until such time as evidence overwhelmingly supports the opposite).
To that end we should at least add this information to the CWP docs.

As a result I don't see why this isn't a part of a very core and required part of the platform specific module code... i.e. https://github.com/silverstripe/cwp-core

robbieaverill commented 6 years ago

I agree that the config should be in cwp-core

robbieaverill commented 6 years ago

It sounds to me like the problem here is that some CWP users aren't using the cwp-recipe-cms, so they're not getting the new config. Since it's an infrastructural level config it should be in cwp/cwp-core, so the PR to remove it from the recipe is https://github.com/silverstripe/cwp-recipe-cms/pull/12 and to add it to cwp-core is https://github.com/silverstripe/cwp-core/pull/49

NightJar commented 6 years ago

It is done :) If this does not resolve the issue @emteknetnz please feel free to re-open.

sunnysideup commented 5 years ago

We have CWP 2.1 installed:

in cwp-recipe-cms:

---
Name: cwpoembedconfig
After: coreoembed
Except:
  environment: dev
Only:
  EnvVarSet: SS_OUTBOUND_PROXY
---
SilverStripe\Core\Injector\Injector:
  # Configure the CWP proxy if defined
  Embed\Http\DispatcherInterface:
    class: Embed\Http\CurlDispatcher
    constructor:
      config:
        # CURLOPT_PROXY = 10004
        10004: '`SS_OUTBOUND_PROXY`'
        # CURLOPT_PROXYPORT = 59
        59: '`SS_OUTBOUND_PROXY_PORT`'

  # Provide dispatcher to Embeddable implementations
  SilverStripe\View\Embed\Embeddable:
    properties:
      Dispatcher: '%$Embed\Http\DispatcherInterface'

so:

not yet in core
not working ;-( despite config being present

We are on

cwp/cwp                                                  2.1.1             
cwp/cwp-core                                             2.1.1             
cwp/cwp-pdfexport                                        1.0.1             
cwp/cwp-recipe-cms                                       2.1.1             
cwp/cwp-recipe-core                                      2.1.1

brettt89 commented 5 years ago

Anyone still encountering this bug (because a new version of CWP has not been released yet), you can fix this by adding the below changes directly to your project's _config/ directory.

https://github.com/silverstripe/cwp-core/pull/49/files

To ensure you don't run into issue when the new CWP version is released, we would recommend giving your copy of this .yml file a unique "Name" (e.g. Name: myoembedconfig).

The reason why this is not working in 2.1.1 is because cwp-recipe-cms does not autoload its _config/ directory so this configuration is never picked up.

sunnysideup commented 5 years ago

Hi Brett,

We had it loaded into our mysite config folder and it still did not work with CWP 2.0 - but working now. Thank you.

silverstripe / cwp-recipe-cms

Re-investigate broken YouTube embed links #11