Closed (fixed)
Project:
Lupus Decoupled Drupal
Version:
1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
11 Nov 2022 at 17:39 UTC
Updated:
14 Dec 2023 at 13:59 UTC
Jump to comment: Most recent
Add responsive preview functionality.
Build solution on top or responsive_preview module.
But alter routes so that it uses CustomElementsController. Add support for the /layout-preview route in lupus_decoupled_layout_builder
Override PreviewUrl method so that it returns urls adjusted to a frontend.
Implements hook_toolbar() to add needed links where need be.
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
fagoThis should add "layout-preview" route, but that would make more sense to add in the layout_builder sub-module
Comment #3
fagoComment #4
fagoIf possible, the integration with responsive-preview module should be optional, so it just works when the module is active, but it's not hard-dependency. The /layout-preview route can always be there when the module is active, I think that makes sense anyway.
Comment #5
roderikTrying to make this work on the base of preexisting Drunomics code. I'm sure the attribution will needs extending.
Comment #6
fagolet's organize code well and add a lupus_decoupled_responsive_preview sub-module for everything that requires responsive-preview.
we decided it's a good idea to do that for all contrib integrations
Comment #7
roderikDraft notes:
Pushing current state of work: draft, as of two weeks ago. Only testing 'regular' preview so far. Maybe I will have an update later today, or maybe not for another two weeks again -- so I should push this now.
I was still testing the first step which is just whether the regular 'non responsive' preview button works, on top of this existing code. (Layout builder related code plus some others are not in my first commit yet.)
The preexisting code also uses the LupusDecoupledPreview class for modifying the 'regular' preview URL.
My interpretation of the pushed preexisting code is:
Update: I managed to get all the above points wrong. Please ignore.
I've already received a review of the general idea, and:
This should be renamed from lupus_decoupled_preview to drupal_decoupled_responsive_preview. The above 'regular node preview' should be moved into lupus_decoupled_ce_api.
It was suggested to do as little modification of Core functionality as possible. That includes trying to not override this 'preview of unchanged node' path. I am not sure yet what implications that has for LupusPreviewPathProcessor which should, AFAIK (new here), still add ?auth=1 to the URL.
Comment #8
roderikCurrent status: 'regular preview' has been moved into lupus_decoupled_ce_api (in the same PR).
There's basically one effect of this added code: the preview redirects to the 'correct' front end, if there are multiple front ends.
(Secondary effect: I actually understand existing code, and which functionality is covered by which parts.)
I believe that now, the
// Node edit form preview url.block ingetPreviewUrl()is unnecessary. It used to be called from the 'Preview' form submit which has been removed in https://git.drupalcode.org/issue/lupus_decoupled-3320802/-/commit/cd667f....Comment #10
roderikThis should be reviewable now;
At several points yesterday, I needed to apply #3395551: FrontendRedirectSubscriber should protect against being hijacked by 'destination' to fix the following:
...however I can neither reproduce it anymore at the moment, nor reason why/how the ?destination would be propagated into the preview. So I'm leaving it that out until I see it again.
Comment #11
fagoThx. Generally the changes seem good, but I think the code organization needs little work. See my above remarks.
> The /layout-preview route can always be there when the module is active, I think that makes sense anyway.
So code for that route should be moved into the layout_builder sub-module, it's not related to responsive preview really, but a generally layout-builder preview route which responsive preview then uses.
ad #3395551: FrontendRedirectSubscriber should protect against being hijacked by 'destination': not sure, but if it's not popping up again I'd agree to not handle it here
Comment #12
roderikAll validation errors should be gone when #3402425: Fix PHPCS and maybe other errors is merged and this is rebased upon it. (Test error still present, as noted there.)
Comment #13
roderikComment #14
arthur_lorenz commentedI found a couple of small issues in the MR
Comment #15
roderikThe PHPUnit error is also present in other issues' builds so that needs to be addressed separately; there's an issue already open for that.
(Waiting for final test results - the latest test build isn't super fast.)
Comment #16
roderikUpdate: latest commit shows PHPstan errors related to not being able to find responsive_preview module:
Drupal\lupus_decoupled_responsive_preview\LupusDecoupledResponsivePreview extends unknown class Drupal\responsive_preview\ResponsivePreview.https://git.drupalcode.org/issue/lupus_decoupled-3320802/-/jobs/379502
So apparently the "test_dependencies:" (whose exact defined use I don't know) does not solve this.
And I'm reinstating the require-dev of drupal/responsive_preview, in order to make tests pass. I believe that require-dev is a good use for that and the module will still not be a requirement on 'regular' site installs.
Comment #17
roderikComment #18
arthur_lorenz commentedThank you, testing previews on a fresh D10.1 with our nuxt kickstarter frontend was successful.
Tested:
Comment #20
fagoMerged, thank you!
Comment #21
roderikThis is so embarrassing (and short) that I'm not going to open a new issue, but instead try to open a new MR here:
While fixing PHPCS/Stan errors.suggestions I copypasted wrong and replaced
strpos($url, '_') !== FALSEwithstr_contains($url, '_')strpos($part, '_') !== FALSEwithstr_contains($url, '_')<======...in ContentSecurityEventSubscriber.
As a result, a hostname containing an underscore in the first part e.g. https://temp_env.site.com is turned into https://*.*.* (instead of https://*.site.com) in the CSP header and your browser says "haha nice joke, I'm not displaying that frame"
.
Comment #24
fagook, strange this did not pop up during testing? Merged.