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

commandURLCleanup now strips all unwanted chars at the end of the string #17

Merged
merged 4 commits into from
Jan 12, 2017

Conversation

pkamps
Copy link
Member

@pkamps pkamps commented Oct 3, 2016

This came up in the CSM project: ticket 3479.

If you have an object name like "about." (including the double quotes), ezp was not removing the dot after 'about'.

"#\.\.+#", # Remove double dots
"#[{$sepQ}]+#", # Turn multiple separators into one
"#^[{$sepQ}]+|[{$sepQ}]+$#" ), # Strip separator from beginning/end
"#^[{$sepQ}\.]+|[{$sepQ}!\.]+$#" ), # Strip unwanted chars from beginning/end
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs to be be more specific. It should say "Strip separator and periods from beginning / end"

* @package tests
*/

class eZCharTransFormTests extends ezpTestCase
Copy link
Member

Choose a reason for hiding this comment

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

the "f" should not be capitalized

public function __construct()
{
parent::__construct();
$this->setName( "eZCharTransFormTests" );
Copy link
Member

Choose a reason for hiding this comment

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

the "f" should not be capitalized

{
parent::__construct();
$this->setName( "eZFile Test Suite" );
$this->addTestSuite( 'eZCharTransFormTests' );
Copy link
Member

Choose a reason for hiding this comment

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

the "f" should not be capitalized

@@ -0,0 +1,41 @@
<?php
/**
* File containing the eZCharTransFormTests class
Copy link
Member

Choose a reason for hiding this comment

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

the "f" should not be capitalized

@pkamps
Copy link
Member Author

pkamps commented Oct 4, 2016

Feedback implemented

@@ -397,7 +397,7 @@ static function commandUrlCleanup( $text, $charsetName )
$text = preg_replace( array( "#[^a-zA-Z0-9_!\.-]+#",
"#\.\.+#", # Remove double dots
"#[{$sepQ}]+#", # Turn multiple separators into one
"#^[{$sepQ}\.]+|[{$sepQ}!\.]+$#" ), # Strip unwanted chars from beginning/end
"#^[{$sepQ}\.]+|[{$sepQ}!\.]+$#" ), # Strip separator and dot from beginning, strip exclemation mark, dot and separator from end
Copy link
Member

Choose a reason for hiding this comment

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

typo: should be "exclamation"

@pkamps
Copy link
Member Author

pkamps commented Oct 5, 2016

Tyop fixed

@peterkeung
Copy link
Member

+1

@pkamps
Copy link
Member Author

pkamps commented Oct 9, 2016

So I added the fix also to commandUrlFixIRI. It's also covered by dedicated tests.

For commandUrlFixIRI I also drop the 'double quote' char. That was not in the original patch from eZ but I still think it's a good idea to remove it from the URL.

@peterkeung
Copy link
Member

This was fixed in the main eZ repo: ezsystems@6e625be

+1 with the double quote strip as well. We should create an additional pull request against the eZ repo with the double quote strip.

@pkamps
Copy link
Member Author

pkamps commented Jan 12, 2017

ez systems pull request:
ezsystems#1274

@pkamps pkamps merged commit 7517c0e into master Jan 12, 2017
@pkamps pkamps deleted the FixUrlCleanup branch January 12, 2017 15:41
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

Successfully merging this pull request may close these issues.

2 participants