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

Handle edge case in graph view for Munin plugins #10127

Merged
merged 1 commit into from Apr 30, 2019

Conversation

Projects
None yet
3 participants
@CirnoT
Copy link
Contributor

commented Apr 19, 2019

Munin plugins have set param in Graph Controller as 'device=x&plugin=y', which works fine for graph.php call, however it does not work as link to graph page, which expects 'device=x/plugin=y'. This commit adds str_replace in blade view for graph to ensure that '&' is replaced by '/' in param.

The reason why this logic is in view instead of controller is that Munin is the only one that uses this particular type of param 'hack' and creating separate variable in controller to fix what is essentially a 'hack' for single case seems unnecessary.

If this is not acceptable, I will update this PR to create separate variable in controller and then use @Isset statement in blade template to decide whether it should use $param or separate variable for link, however in my opinion it would be harder to understand.

This fixes behavior reported on https://community.librenms.org/t/dashboard-munin-graph-links/3626

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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@murrant

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Yeah, I don't think fixing it like this is preferred.

FYI, the reason it doesn't work is probably because the ampersand (&) is escaped to &amp;

@CirnoT

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

No, the & is not escaped in this case, but URL of https://librenms/graphs/device=7&plugin=sensors_power/type=munin_graph/from=1555689592 is invalid as value of sensors_power/type=munin_graph/from=1555689592 does not exist for 'plugin'. Similarly, graphs/device=7/type=munin_graph/from=1555689667&plugin=sensors_power is invalid too it seems, the only working one would be https://librenms/graphs/device=7/type=munin_graph/from=1555689667/plugin=sensors_power and the order of parameters is irrelevant.

@CLAassistant

This comment has been minimized.

Copy link

commented Apr 20, 2019

CLA assistant check
All committers have signed the CLA.

@CirnoT

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

I've changed how graphs are rendered in blade by adding support for multiple parameters via array. Because it seems that Blade doesn't support no-whitespace tag like Twig that I am familiar with, the spacing looks weird in template now but there isn't any other way that I can see that would make Blade recognize @foreach tag and at the same time allow for spacing that won't be rendered in HTML as spaces in URL.

@CirnoT

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

It looks like the changes in PHP file caused old issue on Code Climate to reappear, hope that won't be a problem.

Do let me know if this is okay or you would prefer to use @php (or possibly <?= as it works better with one-liners) to use implode to join array elements instead of @foreach.

Generally Blade is really not very flexible here and makes the issue harder than it is. In Twig I would generally use params|join to simply join array elements with / and & respectively and be done with it.

@CirnoT CirnoT force-pushed the CirnoT:patch-1 branch from 63a2985 to 0430c16 Apr 23, 2019

@CirnoT

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Fixed requested changes. Using implode now and squashed commits.

@murrant murrant added the WebUI label Apr 30, 2019

@murrant murrant merged commit 735a81c into librenms:master Apr 30, 2019

5 of 6 checks passed

codeclimate 1 issue to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@CirnoT CirnoT deleted the CirnoT:patch-1 branch Apr 30, 2019

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

spencerbutler added a commit to spencerbutler/librenms that referenced this pull request May 21, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Jun 29, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.