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

refactor: Moved ifLabel -> cleanPort and updated the usage #6288

Merged
merged 8 commits into from Apr 4, 2017

Conversation

Projects
None yet
5 participants
@laf
Member

laf commented Mar 28, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Fixes: #2703

This should resolve issues with ports that need to have information escaped / encoded before displaying.

Also fixed the notes widget to allow people to put certain html tags in.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 28, 2017

Thank you for submitting a PR @laf! We have found the following @murrant, @crcro and @f0o based on the history of these files to review this PR.

mention-bot commented Mar 28, 2017

Thank you for submitting a PR @laf! We have found the following @murrant, @crcro and @f0o based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 28, 2017

Auto-Deploy finished, Test PR at http://6288.ci.librenms.org or https://6288.ci.librenms.org

Show outdated Hide outdated includes/common.php
if (!isset($purifier)) {
// initialize HTML Purifier here since this is the only user
$p_config = HTMLPurifier_Config::createDefault();
$p_config->set('Cache.SerializerPath', $config['temp_dir']);
foreach ((array)$purifier_config as $k => $v) {

This comment has been minimized.

@murrant

murrant Mar 28, 2017

Member

no need for a cast here, because you are using keys it won't work (keys will be numeric on something cast to an array.

(Update the phpdoc to indicate $purifier_config should only be an array, maybe a note that it is key->value too

@murrant

murrant Mar 28, 2017

Member

no need for a cast here, because you are using keys it won't work (keys will be numeric on something cast to an array.

(Update the phpdoc to indicate $purifier_config should only be an array, maybe a note that it is key->value too

Show outdated Hide outdated includes/common.php
{
/** @var HTMLPurifier $purifier */
global $config, $purifier;
if (empty($purifier_config)) {

This comment has been minimized.

@murrant

murrant Mar 28, 2017

Member

Can you add a comment here, I can't figure out why this is here by looking at the code alone.

@murrant

murrant Mar 28, 2017

Member

Can you add a comment here, I can't figure out why this is here by looking at the code alone.

This comment has been minimized.

@laf

laf Mar 28, 2017

Member

Done, it's so that we can pass on purifier config options at which point we don't want to have munged the html - see html/includes/common/notes.inc.php as an example.

@laf

laf Mar 28, 2017

Member

Done, it's so that we can pass on purifier config options at which point we don't want to have munged the html - see html/includes/common/notes.inc.php as an example.

Show outdated Hide outdated includes/rewrites.php
}
function ifLabel($interface, $device = null)
function cleanPort($interface, $device = null)

This comment has been minimized.

@murrant

murrant Mar 28, 2017

Member

can you add phpdoc here, mostly to signify that both variables should be arrays.

@murrant

murrant Mar 28, 2017

Member

can you add phpdoc here, mostly to signify that both variables should be arrays.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 28, 2017

Auto-Deploy finished, Test PR at http://6288.ci.librenms.org or https://6288.ci.librenms.org

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 29, 2017

Member

I started to test this and it was inconsistent.
For example, look at the ifAlias line on any port overlib link.

I'll have to think on it more.

Member

murrant commented Mar 29, 2017

I started to test this and it was inconsistent.
For example, look at the ifAlias line on any port overlib link.

I'll have to think on it more.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 29, 2017

Member

I have looked at the ifAlias line on port overlib - what are you seeing and what's your sample ifAlias data?

Member

laf commented Mar 29, 2017

I have looked at the ifAlias line on port overlib - what are you seeing and what's your sample ifAlias data?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 30, 2017

Member

I set my sample data to eth0&amp;<ifName> for ifName, ifDescr, and ifAlias

Member

murrant commented Mar 30, 2017

I set my sample data to eth0&amp;<ifName> for ifName, ifDescr, and ifAlias

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 30, 2017

Member

Pushed an update to fix this. I'd already done it on the line above. Basically display() is already run so gives you the clean data but we then add that into the html - which at that stage looks fine, but as this is an overlib popup the html data is reparsed by the browser and & becomes &. Double display() basically fixes this.

Member

laf commented Mar 30, 2017

Pushed an update to fix this. I'd already done it on the line above. Basically display() is already run so gives you the clean data but we then add that into the html - which at that stage looks fine, but as this is an overlib popup the html data is reparsed by the browser and & becomes &. Double display() basically fixes this.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 30, 2017

Auto-Deploy finished, Test PR at http://6288.ci.librenms.org or https://6288.ci.librenms.org

@murrant murrant referenced this pull request Mar 31, 2017

Closed

fix: html characters in port label #6283

2 of 2 tasks complete
Remove ifNameDescr() function
Fix realtime port page
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Apr 4, 2017

Auto-Deploy finished, Test PR at http://6288.ci.librenms.org or https://6288.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Apr 4, 2017

Auto-Deploy finished, Test PR at http://6288.ci.librenms.org or https://6288.ci.librenms.org

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 4, 2017

Member

@laf There is an issue with ajax tables getting double encoded.
eth0&amp;amp;&lt;Alias&gt;

We could remove the cleanPort(), but I don't think that is correct.
Perhaps a way to skip htmlentities when calling cleanPort()?

Edit: nevermind, found the double display() calls.

Member

murrant commented Apr 4, 2017

@laf There is an issue with ajax tables getting double encoded.
eth0&amp;amp;&lt;Alias&gt;

We could remove the cleanPort(), but I don't think that is correct.
Perhaps a way to skip htmlentities when calling cleanPort()?

Edit: nevermind, found the double display() calls.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Apr 4, 2017

Auto-Deploy finished, Test PR at http://6288.ci.librenms.org or https://6288.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Apr 4, 2017

The inspection completed: 15 new issues, 3 updated code elements

scrutinizer-notifier commented Apr 4, 2017

The inspection completed: 15 new issues, 3 updated code elements

@murrant

murrant approved these changes Apr 4, 2017

I think most spots are correct now. Some of these are hard to manually test.

@laf laf merged commit 1bbbaff into librenms:master Apr 4, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:issue-2703 branch Apr 4, 2017

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