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

Compliant query string ampersands in e_url #4067

Merged
merged 3 commits into from
Jan 14, 2020
Merged

Compliant query string ampersands in e_url #4067

merged 3 commits into from
Jan 14, 2020

Conversation

Deltik
Copy link
Member

@Deltik Deltik commented Jan 12, 2020

Motivation and Context

Fixes #4054, a strict HTML standard compliance issue.

Description

e107::url() now creates query strings delimited by & instead of &. & was considered an ambiguous ampersand in older revisions of the HTML standard, so having that flunks HTML compliance tests.

How Has This Been Tested?

A test replicating the issue in #4054 has been added to the unit tests suite.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

Deltik added 2 commits January 13, 2020 00:54
@Deltik
De-indented e107::url() with guard clause
d13fcd4 
Less cognitive complexity now that the e_url.php check is a guard clause
@Deltik
Old HTML ampersand strict compatibility
e62422d 
- FIX: e107::url() now puts "&" in the query string instead of "&" for compliance with the older,
       looser definition of ambiguous ampersands in the HTML specification.
       Fixes: e107inc#4054
- FIX: Typo in comment
- FIX: Clear the core/e107/addons/e_url registry (cache) because if a plugin is installed after that
       cache is initialized, the cache is not updated anymore. The plugin's e_url is therefore not
       loaded, so SEF URLs won't be generated for that plugin until the cache is regenerated.
- NEW: Test for e107inc#4054
- FIX: e_pluginTest::testGetFields() expects the initial condition of the "forum" plugin to be
       uninstalled.
@codeclimate
Copy link

codeclimate bot commented Jan 13, 2020

Code Climate has analyzed commit 4893ea7 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 86.4% (80% is the threshold).

This pull request will bring the total coverage in the repository to 5.4% (0.0% change).

View more on Code Climate.

@Deltik Deltik mentioned this pull request Jan 13, 2020
Closed
@Deltik Deltik merged commit 9488fdc into e107inc:master Jan 14, 2020
@Deltik Deltik deleted the fix-4054 branch January 14, 2020 09:17
@CaMer0n
Copy link
Member

CaMer0n commented Jan 14, 2020

@Deltik Is this still going to function?

One example: forum_class.php Line 204
$url = e107::url('forum','index','full');
e107::getRedirect()->go($url);

@Deltik
Copy link
Member Author

Deltik commented Jan 14, 2020

@CaMer0n: redirection::go() already handles & in the URL. This was introduced in 96108c3 (2014-08-05). There should be no change in behavior by that function.

When I looked at usages of e107::url(), I found that most usages splice its output directly into <a href="…"></a>, or the output somehow/indirectly makes its way to an HTML entities display.

I've clarified the documentation of the behavior of e107::url() in 82b2da4 and reimplemented the fix for #4054 so that the direct-to-HTML output is now escaped by htmlspecialchars().

@CaMer0n
Copy link
Member

CaMer0n commented Jan 14, 2020

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

& vs &amp; in e_url addon and validation
2 participants
@Deltik @CaMer0n