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

DS-4135 prevent escaping for citation_ tags #2317

Merged
merged 2 commits into from
Jul 19, 2021

Conversation

bram-atmire
Copy link
Member

@bram-atmire bram-atmire commented Jan 5, 2019

@tdonohue tdonohue added this to the 6.4 milestone Jan 7, 2019
@tdonohue tdonohue added interface: XMLUI (obsolete) XMLUI in DSpace versions 1.x through 6.x. Removed in 7.x bug component: SEO Related to Search Engine Optimization labels Jan 7, 2019
@tdonohue
Copy link
Member

tdonohue commented Jan 7, 2019

@bram-atmire : This looks like a good start, though I see the changes are only being applied to Mirage 2 XMLUI theme. I'm assuming we'd need similar/identical changes to Mirage 1? Also, have we verified this does NOT occur in the JSPUI?

@bram-atmire
Copy link
Member Author

Hi @tdonohue - apologies for not catching your comments sooner.

  1. Correct, I think Mirage 1 is also affected
  2. Couldn't reproduce this on JSPUI: http://demo.dspace.org/jspui/handle/10673/142

@bram-atmire
Copy link
Member Author

Just added the fix in for Mirage 1.

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Code looks good to me. I haven't tested it though, so it may be good to do a quick sanity check test prior to merging.

@alanorth
Copy link
Contributor

alanorth commented Feb 26, 2020

Tested and confirmed working on 6.4-SNAPSHOT with an item using non-ASCII Vietnamese text.

Before:

<meta content="Thu hoạch v&agrave; bảo quản c&agrave; ph&ecirc; ch&egrave; đ&uacute;ng kỹ thuật (Harvesting and storing Arabica coffee)" name="citation_title">

After:

<meta name="citation_title" content="Thu hoạch và bảo quản cà phê chè đúng kỹ thuật (Harvesting and storing Arabica coffee)" />

@tdonohue tdonohue added the quick win Pull request is small in size & should be easy to review and/or merge label Feb 26, 2020
@toniprieto
Copy link
Contributor

I think it need some changes. The quote character ("), and others characters, should be escaped.

Testing it with the title:

Test "quoted text" example

Shows an incorrect output:

<meta name="citation_title" content="Test "quoted text" example" />

@alanorth
Copy link
Contributor

alanorth commented Jun 21, 2021

@toniprieto perhaps we should recommend merging this, as it fixes the citation tags in some corner cases (ie Vietnamese text), and then open a new Jira issue for the incorrect quoting? At this rate I think we need to prioritize getting as many fixes into DSpace 6.4 as possible so we can release it ASAP. What do you think, @J4bbi?

The amount of energy going into preparing DSpace 7 is really killing the DSpace 6.x effort—to say nothing of DSpace 5.x—for several years now!

@J4bbi
Copy link
Contributor

J4bbi commented Jun 24, 2021

@alanorth , I think you are right, filing a separate issue seems like a good solution considering, ideally we would want to get 6.4 out this year

@kshepherd , what do you think? I've not tested

@alanorth
Copy link
Contributor

@J4bbi I just verified that the quotation bug is present in vanilla DSpace 6.3, so we should definitely merge this and open a new issue for that. This pull request correctly fixes non-ASCII text in the citation tags.

@J4bbi
Copy link
Contributor

J4bbi commented Jul 17, 2021

@alanorth , thanks

I'll merge this, would you mind opening an issue for the quotation bug?

@J4bbi J4bbi closed this Jul 19, 2021
@J4bbi J4bbi reopened this Jul 19, 2021
@J4bbi J4bbi merged commit e0e7a2a into DSpace:dspace-6_x Jul 19, 2021
@alanorth
Copy link
Contributor

@alanorth , thanks

I'll merge this, would you mind opening an issue for the quotation bug?

Thanks. No problem, I've opened DS-4585: Incorrect escaping of citation_ meta tags when metadata contains quote characters.

BTW how do I get edit permissions on Jira? The formatting is messed up in my bug and I can't edit it.

@J4bbi
Copy link
Contributor

J4bbi commented Jul 21, 2021

thanks

BTW how do I get edit permissions on Jira? The formatting is messed up in my bug and I can't edit it.

@alanorth, I think when you're logged in there should be an edit button => https://jira.lyrasis.org/secure/EditIssue!default.jspa?id=38071

@alanorth
Copy link
Contributor

thanks

BTW how do I get edit permissions on Jira? The formatting is messed up in my bug and I can't edit it.

@alanorth, I think when you're logged in there should be an edit button => https://jira.lyrasis.org/secure/EditIssue!default.jspa?id=38071

I don't see a link. But when I use your link it says:

You do not have permission to edit issues in this project.

@J4bbi
Copy link
Contributor

J4bbi commented Jul 23, 2021

@bram-atmire, thanks for this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component: SEO Related to Search Engine Optimization interface: XMLUI (obsolete) XMLUI in DSpace versions 1.x through 6.x. Removed in 7.x quick win Pull request is small in size & should be easy to review and/or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants