Closed (fixed)
Project:
Preview Link
Version:
2.1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
3 Jul 2020 at 11:53 UTC
Updated:
1 Apr 2024 at 22:35 UTC
Jump to comment: Most recent, Most recent file
Show commands
changes
Comments
Comment #2
arvind.kinjaComment #3
arvind.kinjaPatch file to configure the expiry days from the site backend.
The configuration form can be found at /admin/config/content/preview-link
Comment #4
arvind.kinjaComment #5
arvind.kinjaComment #6
megha_kundar commentedComment #8
dpiWe're not looking to have this for the 1.x branch. Please dont change the version.
Almost all of the patch is unusable since it doesnt actually address any of the configuration of Preview Link.
- It doesnt expose a UI for any existing config, it tries to introduce NEW config.
- Permission is too generic
- Copy paste issues from other projects
Setting back to Active since the patch to date should be mostly ignored.
Comment #10
nterbogt commentedI've redone the permission form configuration. Also moved it to a merge request.
I still need to implement the bundle configuration, but the rest is done, as far as I know.
Comment #12
nterbogt commentedThis is ready for a review please.
Comment #13
acbramley commentedLet's rebase this against 2.1.x.
I'm not sure what the 2.x branch was created for but it's effectively dead.
Comment #14
acbramley commentedComment #15
acbramley commentedMy bad, looks like it already is!
Comment #16
acbramley commentedThere's some fairly complex logic in that form that needs tests. Also pushing some linting updates.
Comment #17
nterbogt commentedI agree with the complex logic in the form, I'm also not really sure about a decent approach to test that. Do you have any recommendations?
A unit test that just checks the default values and possible options could work, but might not be what you're after.
Comment #18
acbramley commented@nterbogt a unit test isn't going to test the form. It should be a functional test that submits the form and checks the config after the fact.
Comment #19
nterbogt commentedAdded a functional test for the settings form. Has a phpstan warning, but it's the same warning in a lot of other projects.
https://git.drupalcode.org/project/token/-/jobs/275660
The warning is documented here.
https://www.drupal.org/docs/develop/development-tools/phpstan/handling-u...
Comment #20
larowlanLeft a couple of questions on the MR
Comment #22
acbramley commentedThanks all.
Comment #24
stephen-cox commentedGreat to see an admin UI for this. Any chance of a new release including this?
The admin UI is useful for use as part of this issue https://github.com/localgovdrupal/localgov/issues/600. As an interim I've created a PR to use the dev branch (https://github.com/localgovdrupal/localgov/pull/691), but would be nice if we didn't have to reference specific commits in the dev branch.
Comment #25
acbramley commented@stephen-cox https://www.drupal.org/project/preview_link/releases/2.1.0-alpha3 :) thanks for prompting!