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

Fix PHP notices #234

Closed
wants to merge 2 commits into from
Closed

Fix PHP notices #234

wants to merge 2 commits into from

Conversation

dregad
Copy link
Contributor

@dregad dregad commented Jun 26, 2013

During tests following conversion of the old MantisBT authentication back-end to a plugin, I realized that DokuWiki triggers a number of PHP notices.

Caused by accessing undefined index REMOTE_USER.
Failure to do this causes a PHP notice error, which can happen e.g. if a
plugin starts a session.
@dregad
Copy link
Contributor Author

dregad commented Jun 26, 2013

Hi Andreas,

Thanks for your feedback and guidance. Having the session opening is a good idea on principle, but it probably would not work for my specific case since the session is opened by the MantisBT API, which has to be included for the auth plugin to work.

So, on second thoughts, maybe what I need to do is tweak the auth plugin to selectively call the required MantisBT APIs instead of simply including all of them through the main core.php; that automatically starts a session, but in fact it's probably not needed for what the plugin needs to do. I'll think about it.

Do you have any comments or objections about the other commit ? If not, do you want to cherry-pick that, or should I submit a separate pull request ?

In any case, I think this one can be closed.

@Chris--S
Copy link
Collaborator

Chris--S commented Aug 3, 2013

I think the intention is correct, but the implementation above looks flawed. E.g. in auth.php it looks like your shifting the problem from $_SERVER['REMOTE_USER'] to $id and $rest. In this instance, maybe ensuring $_SERVER['REMOTE_USER'] is always initialized might be a better solution.

I'll close this PR, but we would accept something similar with a better implementation. Killing notices is good.

@Chris--S Chris--S closed this Aug 3, 2013
@Chris--S
Copy link
Collaborator

Chris--S commented Aug 3, 2013

my above comment isn't strictly correct :)

however, in checking the code referenced in auth.php, we've discovered a number of flaws...

  • rules should be skipped when they contain %USER% tokens and there is no logged in user (as happens for %GROUP%).
  • code needs to be reorganized to allow for rules which contain both %GROUP% and %USER% tokens.

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.

None yet

3 participants