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

Entry filter not working with titles containing Ampersands #2446

Closed
animaux opened this issue May 20, 2015 · 34 comments
Closed

Entry filter not working with titles containing Ampersands #2446

animaux opened this issue May 20, 2015 · 34 comments
Milestone

Comments

@animaux
Copy link
Contributor

animaux commented May 20, 2015

Symphony 2.6.1

A similiar problem to this JS-related problem:

When I try to filter a list of entries in in index view by a regular input field with a value containing an ampersand, the value reverts to only the word before the ampersand. Here are my steps:

  1. create a new filter in index view, and filter by input field title
  2. filter by value is and select a value from the pop-up of existing values f. e. »Bilden & Betten«
  3. the filter reverts to »Bilden« and the result thus is no entry

Probably the ampersand needs to be escaped/encoded at some point and isn’t. Typing »Bilden & Betten« by hand instead of clicking the existing value yields the same result.

@michael-e
Copy link
Member

Reminds me of #2349. (But in that case escaping with a backslash helped. I don't know what's the solution for ampersands.)

@michael-e
Copy link
Member

Ah, got it: Replacing the ampersand by %26 in the URL works. So it can probably be solved similar to #2349.

@animaux
Copy link
Contributor Author

animaux commented May 20, 2015

Brilliant, sounds good! Thanks Michael.

@michael-e
Copy link
Member

Something like adding value.replace('&', '%26'); after this line. Could you test that, @animaux ?

@michael-e
Copy link
Member

Ah, sorry, again we'd only replace the first occurence with that…

@michael-e
Copy link
Member

See #2420. We must either split the string or use regex. @nitriques suggested regex last time, but we kept the split in the end. Time to give it another thought. We should make it consistent.

Solution 1:

input.val(value.split(',').join('\\,'));
input.val(value.split('&').join('%26'));

Solution 2:

input.val(value.replace(/,/g, '\\,'));
input.val(value.replace(/&/g, '%26'));

@nitriques, @brendo?

@brendo
Copy link
Member

brendo commented May 20, 2015

Nah, this is getting a little hacky. We should fully encode the values using encodeURIComponent which will handle all special characters.

@michael-e
Copy link
Member

Unfortunately, that doesnt' work, because it will encode , as %2C. For some reason we need to escape this as \,.

The output of

encodeURIComponent('test1, test2 & test3');

is test1%2C%20test2%20%26%20test3, but we need test1\,%20test2%20%26%20test3 for the filter to work correctly.

@brendo
Copy link
Member

brendo commented May 20, 2015

What a bloody mess. You are correct, we need \, because the
Datasource::determineFilterType looks for the specific pattern of commas
escaped by backslashes.

Perhaps the Q&D method is the best way after all.

On Wed, May 20, 2015 at 8:20 PM, michael-e notifications@github.com wrote:

Unfortunately, that doesnt' work, because it will encode , as %2C. For
some reason we need to escape this as \ ,.

The output of

encodeURIComponent('test1, test2 & test3');

is test1%2C%20test2%20%26%20test3, but we need
test1,%20test2%20%26%20test3 for the filter to work correctly.


Reply to this email directly or view it on GitHub
#2446 (comment)
.

@michael-e
Copy link
Member

Which route? Split or regex replace? (I am by no means an expert in JS.)

@brendo
Copy link
Member

brendo commented May 20, 2015

Either or, something like the following should do the trick:

value = value.replace(/,/g, '\\,').replace(/&/g, '%26');
input.val(value);

@michael-e
Copy link
Member

Looks good to me, but needs testing nevertheless. Shall we pass the testing job to @animaux? :-)

It's this line:

input.val(value.split(',').join('\\,'));

which would be replaced by these two lines:

value = value.replace(/,/g, '\\,').replace(/&/g, '%26');
input.val(value);

@animaux
Copy link
Contributor Author

animaux commented May 20, 2015

Ok, did that. Do I have to recompile symphony.min.js or is this done on-the-fly? In case not, how do I do this?

@michael-e
Copy link
Member

Ah, I just noticed that it's a bit hard to test, because Symphony sends the minified JS file…

I have no idea how to recompile the minified file ("correctly"). Last time I hacked into the minified file in order to test the stuff.

@brendo
Copy link
Member

brendo commented May 20, 2015

Actually that's a good point, @nilshoerrmann @nitriques are we able to write up a few lines about how to install/recompile the minified assets and add them to the Contributing guide?

@animaux It's done using grunt, but I'm not super familiar with the full steps to get grunt installed. I think it's just a npm install from the root directory (to get the dependencies) and then run grunt to actually perform the tasks.

@michael-e
Copy link
Member

Once again I hacked the minified file. I replaced .split(",").join("\\,") with .replace(/,/g, '\\,').replace(/&/g, '%26'), and it worked fine. So we should be fine with @brendo's code!

@jonmifsud
Copy link
Member

@brendo those should be the steps. I've set it up to do some modifications before.

@animaux
Copy link
Contributor Author

animaux commented May 20, 2015

@michael-e I’m not able to find .split(",").join("\\,") in my symphony.min.js. Symphony 2.6.2 master. Are you in the integration branch?

@michael-e
Copy link
Member

No, I am on Symphony 2.6.2. I can find the string. Which editor do you have?

@animaux
Copy link
Contributor Author

animaux commented May 20, 2015

BBEdit 11.1

@animaux
Copy link
Contributor Author

animaux commented May 20, 2015

m( d’oh: hadn’t reloaded the file in the editor after the 2.6.2 update … found it.

@nilshoerrmann
Copy link
Contributor

Actually that's a good point, @nilshoerrmann @nitriques are we able to write up a few lines about how to install/recompile the minified assets and add them to the Contributing guide?

@nitriques might be better at this job :)

@animaux
Copy link
Contributor Author

animaux commented May 20, 2015

Work’s fine for me! Tested with:

  • Regular Characters + Spaces
  • Ampersand
  • Umlaut (ä, ö, ü)
  • Comma

Cheers!

@brendo brendo added this to the 2.6.3 milestone May 22, 2015
@nitriques
Copy link
Member

@brendo @nilshoerrmann
Sorry it took so long, but, here it is: https://github.com/symphonycms/symphony-2/wiki/Building-the-client-assets-files

I've also added a link to it on the home page.

Is it missing something ?

@brendo
Copy link
Member

brendo commented May 23, 2015

Pushed the fix for real :)

@brendo brendo closed this as completed May 23, 2015
@michael-e
Copy link
Member

The page is completely gone: https://github.com/symphonycms/symphony-2/wiki/Building-the-client-assets-files

I know it was there, I saw it. How can that be?

@michael-e
Copy link
Member

Re: The fix: Confirmed that it works as advertised. ( @animaux )

@brendo
Copy link
Member

brendo commented May 23, 2015

Sorry, that was me, I thought Github pages did automatic redirects, https://github.com/symphonycms/symphony-2/wiki/Building-Symphony%27s-Assets

@michael-e
Copy link
Member

Ah, thanks!

@nitriques
Copy link
Member

Oh sorry for the typo!

And is the doc ok ?

@michael-e
Copy link
Member

Yeah, sure! I just had to do install grunt-cli as well (npm install -g grunt-cli) in order to make it work for me.

@nitriques
Copy link
Member

Totally true. It's not grunt that you need to install globally, it's the cli.

I will update the doc accordingly. Thanks

@animaux
Copy link
Contributor Author

animaux commented May 27, 2015

@michael-e Prima, thanks! :)

@michael-e
Copy link
Member

You're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants