This would be a small usability improvement. Right now, when you add a REST export display, you immediately get an error saying a path hasn't been specified. You then need to manually specify a path, and it's not clear as an end user a) where that is, and b) what you'd put in there. The flow is kind of like this:
Suggestion would be that if the View already has a Page display (such as the Frontpage view), just carry over that path so the user never gets this error in the first place. This would mirror how entities work (what you get on node/1 depends on what headers you sent). And in my testing, if you specify /node as the REST export path of the Frontage view, and shoot text/html you get back HTML, if you shoot application/json+hal, you get back JSON, so it seems to work as expected.
Comment | File | Size | Author |
---|---|---|---|
#33 | 2302615-33.patch | 6.76 KB | _utsavsharma |
#32 | reroll_diff_15-32.txt | 7.45 KB | sahil.goyal |
#32 | 2302615-32.patch | 6.73 KB | sahil.goyal |
#15 | 2302615-15.patch | 6.99 KB | clemens.tolboom |
#8 | 2302615-7.patch | 6.48 KB | dawehner |
Comments
Comment #1
dawehnerIf your rest display should work like a page display use the clone display as rest export functionality.
People will disagree with that statement, but I really think this is a feature request not a task. Hard coding everything is a bad behavior,
given that in order to make the path usefull really working you basically want to have the same filters, fields, contextual filters and access configuration aka clone.
Comment #2
dawehnerAn alternative, which at least improve things, would be to automatically open the path modal when you add a new rest display.
Comment #3
dawehnerComment #4
clemens.tolboomComment #6
Helpermedia CreditAttribution: Helpermedia commentedPatch is re-rolled. It's not working as expected, cause is probably:
The path modal does not come up as expected according to #2
Comment #7
clemens.tolboomThis does not seem trivial according to @dawehner@irc
Checking addFormToStack
it suggests we have an overlay but that's not true. We just clicked the 'Add REST export' button.
Comment #8
dawehnerWorked on it, total different approach.
Comment #9
clemens.tolboomNice rephrase solution. Works as described in #2
This solved the required path for (rest export) path displays by adding a dialog but what if two or more fields are required? The patch from #3 suggested we can add multiple items to the form stack.
I like the solution of #8 but let @webchick RTBC
Maybe @Helpermedia can do another review?
Comment #10
Wim LeersComment #11
Wim Leers@dawehner asked me to review this, but I don't feel qualified to review this. I think @tim.plunkett would be a better candidate. (None of this even touches the REST module btw: 0 changes in
\Drupal\rest\Plugin\views\display\RestExport
.)Comment #12
clemens.tolboom@wim-leers changing the title doesn't fix the 0 rest related changes ;-)
This patch pops-up the path dialog for any page based display.
Comment #13
dawehner@Wim Leers
Well, the reason to review that was related with the javascript additions ...
Comment #14
dawehnerDon't we want that all the time? Seems like a super reasonable assumptions for me at least.
Comment #15
clemens.tolboomPatch needed a re-roll. I took a3fd4d0f8cf5c5c as a start.
Adding a new (path based) display does not trigger the path dialogue so needs work too.
Comment #16
vbouchetHaving this improvement may fix a real bug.
Comment #17
dawehnerJust some quick nitpick review :)
unrelated change
Let's put
{@inheritdoc}
on thereComment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedCurious what the subsystem maintainer things? Also some changes were requested in last comment
Comment #32
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedi'm rerolling the patch as seen that #15 was too old so it cannot apply to the current version 10.1.x, addresses the comment #17. And attaching the reroll file for the same.
Comment #33
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedFixed #32.
Please review.