Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: FIX Existing URL query strings should be preserved #824

Conversation

ScopeyNZ
Copy link
Contributor

Currently when adding an upload field to the CMS fields of a versioned object it's possible that the "createFileEndpoint" may be provided with URL query params - eg. from VersionedStateExtension. This PR makes sure any existing query is preserved (and prevents things like /my/base/link?stage=Stage/2).


if (schemaUrl.indexOf('?') !== false) {
const questionMarkIndex = schemaUrl.indexOf('?');
urlQueryString = schemaUrl.substring(questionMarkIndex + 1) + (urlQueryString || '');
Copy link
Contributor Author

@ScopeyNZ ScopeyNZ Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's potential for a missing & here.

@ScopeyNZ ScopeyNZ force-pushed the pulls/1.2/united-states-of-versioned branch from 12334ac to fc09101 Compare August 22, 2018 22:01
@ScopeyNZ
Copy link
Contributor Author

I think this problem isn't limited to asset admin - as I'm having a very similar issue with recent history viewer work in elemental. Perhaps this is not the right place to fix this. I'm going to mark this as WIP until I figure out what's going on.

@ScopeyNZ ScopeyNZ changed the title FIX Existing URL query strings should be preserved WIP: FIX Existing URL query strings should be preserved Aug 26, 2018
@robbieaverill
Copy link
Contributor

Yeah, just reproduced on elemental as well. I think this could be a problem solved outside of this module

@robbieaverill
Copy link
Contributor

It's worth noting that the backend counterpart for this code isn't really expecting to return any query strings, so we could probably just strip them off and ignore them. It'd be much better to return data in the schema body than in the URL for it

@robbieaverill
Copy link
Contributor

Related: silverstripe/silverstripe-admin#607

@blueo
Copy link
Contributor

blueo commented Sep 25, 2018

FWIW I'm experiencing this behaviour when using Better Navigator. It provides an 'edit in CMS' button that appends a stage param to it's link (eg admin/edit/show/2?stage=Stage when then creates the invalid URL above. There seem to be a number of places where the JS isn't checking/expecting URL params. Perhaps it would be good to have a standard JS lib for handling urls and preserving/ignoring params

@ScopeyNZ
Copy link
Contributor Author

This should be fixed by silverstripe/silverstripe-versioned#173 now.

@ScopeyNZ ScopeyNZ closed this Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants