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.

Sign up for GitHub

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

Jump to bottom

EZP-31248: Fixed handling non-printable characters for Search #2904

Merged
merged 10 commits into from
May 6, 2020
Merged

EZP-31248: Fixed handling non-printable characters for Search #2904

merged 10 commits into from
May 6, 2020

Conversation

hgiesenow
Copy link
Contributor

@hgiesenow hgiesenow commented Dec 19, 2019 •
edited by andrerom
Loading

Question Answer
JIRA issue EZP-31248
Bug/Improvement yes
New feature no
Target version 7.x
BC breaks no
Tests pass ?
Doc needed no

Replaces all special chracaters to avoid issues with SolR

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@andrerom
Copy link
Contributor

andrerom commented Jan 8, 2020 •
edited
Loading

@kmadejski is this overlapping with the PR you proposed perhaps?
If it makes sense to skip these things in kernel, I'm all for it as it makes sure search engines indexes the same.

ref: ezsystems/ezplatform-solr-search-engine#159

@kmadejski
Copy link
Member

@hgiesenow thanks for your contribution!

@andrerom yes, it seems to cover also what I've tried to achieve in my PR. This is also probably what @gggeek had in mind in ezsystems/ezplatform-solr-search-engine#159 (comment).

@hgiesenow could you please double-check whether it for sure covers what has been described in this ticket: https://jira.ez.no/browse/EZP-30759?

@gggeek
Copy link
Contributor

gggeek commented Jan 27, 2020 •
edited
Loading

Maybe :-) I don't remember the details.

Otoh I think that some of the chars that are trimmed-away at the moment (both before and with this PR) should not be replaced with no-char but with a space instead.

Esp. tab, vertical-tab, form-feed could conceivably be used to separate two words. If we strip them out, we'd be joining those words together... possibly chars 1C-1E as well

@mateuszbieniek
Copy link
Contributor

Hi @hgiesenow, do you need any help with this PR? 🙂

@mateuszbieniek mateuszbieniek requested a review from andrerom April 21, 2020 11:16
@mateuszbieniek
Copy link
Contributor

Hi @kmadejski, @gggeek - I pushed a commit that handles tabs, horizontal tabs and form feed, so they are replaced with space.

Can you please give your review (for some reason I can't add you to reviewers)?

@gggeek
Copy link
Contributor

gggeek commented Apr 21, 2020

better :-)

@mateuszbieniek mateuszbieniek self-assigned this Apr 21, 2020
andrerom
andrerom approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@andrerom andrerom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1, but should go to 6.13 and up on merge

@andrerom andrerom requested review from alongosz, mateuszbieniek and adamwojs and removed request for mateuszbieniek April 21, 2020 14:33
adamwojs
adamwojs reviewed Apr 21, 2020
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

+1 but I'm missing tests 😉

@mateuszbieniek
Copy link
Contributor

mateuszbieniek commented Apr 22, 2020 •
edited
Loading

@adamwojs had some problems with those, but should be ok now 😉

@alongosz alongosz changed the title EZP-31248 Remove all non-printable characters EZP-31248: Fixed handling non-printable characters for Search Apr 22, 2020
@tomaszszopinski tomaszszopinski self-assigned this May 6, 2020
tomaszszopinski
tomaszszopinski approved these changes May 6, 2020
View reviewed changes
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on eZ Platform EE 2.5 with diff.

@alongosz alongosz merged commit e4e7942 into ezsystems:7.5 May 6, 2020
@alongosz
Copy link
Member

alongosz commented May 6, 2020

Merged for eZ Platform v2.5.x via e4e7942 and for eZ Platform v3.0 via ezsystems/ezplatform-kernel@9e3ef6e.

Thank you @hgiesenow and @mateuszbieniek 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszbieniek mateuszbieniek mateuszbieniek left review comments

@andrerom andrerom andrerom approved these changes

@adamwojs adamwojs adamwojs left review comments

@tomaszszopinski tomaszszopinski tomaszszopinski approved these changes

@kmadejski kmadejski kmadejski approved these changes

@alongosz alongosz alongosz approved these changes

Assignees

@mateuszbieniek mateuszbieniek

@tomaszszopinski tomaszszopinski

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hgiesenow @andrerom @kmadejski @gggeek @mateuszbieniek @alongosz @adamwojs @tomaszszopinski