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

replacing regex to un-break groups menu on emonpi #62

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

shadowguardian507-irl
Copy link

@shadowguardian507-irl shadowguardian507-irl commented Dec 28, 2019

I have refactored the code to use the formal php mysql string sanitisation functions and removed the broken regex expressions that were blocking successful creation and edit of groups
(group would be made but would be called null with a null description as the regex seemingly blocked all strings of any type)

I have tested this on an emonpi unit and it is working ok in regards to groups.

there are still related problems though see issue #63

lets see what the get statements are up to
missing ;
hope the bug is here
place to put notes on potential issues/fixes that may be needed
seems that ini settings are not being honoured, why else would using the group specific feed bits result in errors like this in the log
2019-12-28 17:24:42.506|WARN|PHPFina.php|get_meta() meta file does not exist '/var/lib/phpfina/8.meta'
(setting file has, phpfina[datadir] = '/var/opt/emoncms/phpfina/')
however it looks like it already does load the file, which raises even more confusion as to why it picks the wrong path 😟
@TrystanLea
Copy link
Member

Hello @shadowguardian507-irl

Thanks a lot for this, sorry its taken me almost a year to review!
I think using real_escape_string is too permissive, I wanted to restrict the possible characters more strictly. I've modified the regex to similar to other emoncms modules:

    $group_name = preg_replace('/[^\p{N}\p{L}_\-:]/u', '', $group_name);

This seems to be working fine.

Thanks for the pointers and comments with your pull request it helped a lot to understand the issue!

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