-
Notifications
You must be signed in to change notification settings - Fork 5
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
v0.6.4: Now works under PHP 7, with more warnings fixed #13
Conversation
debian/changelog updated Added lib/ containing tweaked version of pear/auth www/PLUG/accesscheck.inc.php: + `check_level()`: Can now handle if `$user_details['memberOf']` is a single value www/PLUG/pagefunctions.inc.php: + Added `UnauthorisedException` + `display_page()` can now handle `generate_menus()` throwing an exception + `generate_menus()` throws UnauthorisedException instead of returning false when not logged in + Fixed menu labels All the above .php files: + Fixed space-at-end-of-line + Made statement parentheses consistent + Added Vim & Emacs tab width settings (don't worry, it uses spaces!) www/templates/loginform.tpl: + $error is conditionally shown + Used CSS classes for improved error highlighting: `ui-widget` `messagewidget` `ui-state-error` `ui-corner-all` `ui-icon` `ui-icon-alert` Tweaked .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few changes. One observation, we appear to have lots of extra files pulling in the pear package, instead of just the pear package. (lock files, channels etc). Are they needed? Would composer be a better way of pulling this in. It wouldn't take much to move this to composer autoloader, including for our PLUG classes?
@@ -1,14 +1,19 @@ | |||
<?php | |||
|
|||
if(!isset($pagestarttime)) // For pages that don't need auth | |||
class UnauthorisedException extends RuntimeException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is old code, we should try and stick with modern best practices. Classes should be defined in their own files
Also, just to be clear. I'm slightly horrified that this code is still in use. I don't have time to maintain or update it, so any updates you guys do, I'm OK with. Given that my day job involves lots of code review and trying to get things up to modern standards, that's unfortunately how I'll review changes, but that doesn't mean you actually need to listen to my changes, I fully support you guys hack fixing it as much as you want. Thanks for putting effort in to make it work, your effort is appreciated. |
This is not a full review, but one thing that sticks out to me is the copy/paste of PEAR::Auth, along with state files for the pear package manager. This obscures the other changes you've made, and it is not obvious where the code comes from. If we are only pulling in PEAR::Auth, would it be possible to reference via a git submodule? Looking at the commit history, I see you needed to make a fix to the LDAP backend's fetchData method. So the submodule would probably need to point at a fork of https://github.com/pear/Auth. We can put that fork under the plugorgau namespace if you'd prefer. |
For those commenting that the code for PEAR::Auth has been copy-pasted, this would be a pretty basic gaffe if done without consideration. However, due to the fact that this is a deprecated PEAR module and the code is no longer available in Git repo anywhere, an alternative had to be found. Also, installing from the PEAR servers is broken for this module. Copying the source into the repo was the only option left. |
@unixnut It would be good to split out the pear stuff into a submodule or something. I understand the need to have the code accessible (and possibly forked), but it being here in this repo makes it harder to see the actual changes. With a git submodule, we could still include it in the single deb package that is built, but the code could live in its own repo in the plugorgau namespace. If you want help splitting it out, I'm happy to lend a hand with that. |
So after a bit of investigation, it looks like we can probably go with upstream pear::Auth as is. According to https://bugs.php.net/bug.php?id=13892 it seems additional function arguments are simply ignored by design. While it would certainly be cleaner for each overload to have the same signature, having no diff to upstream reduces the maintenance burden. |
extras/README.md: + Added System package sections + Added "Web server configuration" section + Page title + Other tweaks
debian/plug-ugmm.install: + Changes the way files are installed into /usr/share/doc/plug-ugmm/; the contents of the extras/ subdirectory is installed, instead of the subdirectory itself debian/control: + Depends on 'nginx' or 'apache2-bin' package + Depends on 'php' virtual package instead of 'php7.0' + Recommends 'php-fpm' virtual package debian/postinst: script no longer uses Apache commands or initscripts Added debian/postrm: + Removes /usr/share/plug-ugmm/www/templates_c/ README.md: Moved "System package installation" section Moved groups.ldif to extras/examples/ for consistency and so it will get installed by the package www/PLUG/Members.class.php: + `Person::change_forward()`: Avoids warning if `$this->userldaparray['mailForward']` isn't set + `Person::update_ldap()`: Avoids warning from nested arrays + Converted tabs to spaces www/PLUG/session.inc.php: + `loginForm()`: Fixed typo with `$error` www/templates/editmember.tpl: + Allows for `$member.mailForward` to be absent www/templates/editselfdetails.tpl: + Allows for `$member.pager` to be absent www/templates/editselfforwarding.tpl: + Allows for `$member.mailForward` to be absent www/templates/listusers.tpl: + Allows for `$user.mailForward` to be absent www/templates/memberself.tpl: + Allows for any of `$memberself.mobile`, `$memberself.pager` or `$memberself.homePhone` to be absent + Fix for `$memberself.mailForward` check Added extras/examples/nginx/plug-ugmm.section.conf Removed old plug-ugmm.conf Added www/{favicon.ico,images/logo.png}
Hi, all. I have completed most of the work listed in the RFT2019-02-WO1 Work Order. This has been built into a package and tested. See v0.6.2 and v0.6.3 in debian/changelog (attached here as changelog_2020-04-23.txt) for a concise overview of the new changes. |
Can we please either put the pear stuff in another git repo that we can pull from, or check if we can use composer to pull from upstream. It would make the ugmm repository cleaner. |
debian/changelog updated www/plug/pagefunctions.inc.php: + utility defines for `display_page()` args: `use_header`, `no_header`, `use_footer`, `no_footer` + only sets 'title' smarty var if `$title` is set - this tells header.tpl to not display the header block www/plug/pagefunctions.inc.php: + `$submenu['ctte']['expiredmembers']`: added 'label' element to avoid error in `generate_menus()` www/plug/session.inc.php: + `loginform()`: `$title` is now a global, and is set to the empty string when showing the login page, i.e. loginform.tpl + custom `$pagetitle` suffix for the login page, i.e. loginform.tpl + 'usercreated' smarty var now defaults to false www/ctte-newmember.php: + `$_get['expiredmembers']` can now be safely absent www/resetpassword.php: + 'resetform' and 'successform' smarty vars now default to false + custom `$pagetitle` suffix for the page, i.e. resetpasswordform.tpl www/resendack.php: + added `display_page()` vars: `$access_level`, `$toplevel`, `$pagetitle`, `$title` www/signup.php: + custom `$pagetitle` suffix for the page, i.e. signup.tpl www/templates/footer.tpl: + fixed closing diff comment for #content + added closing diff for #page www/templates/header.tpl: + smarty `if` block around "header block" (h2 element and includes for menu.tpl & messages.tpl); this lets pages using this template leave `$title` unset to tell this template not to show the header block, e.g. if their specific template has its own header block (with h2 and includes as necessary) + 'title' smarty var can be absent without error thanks to `default` modifier (without params, this modifier supplies the empty string) www/templates/resetpasswordform.tpl: + includes messages.tpl given that header.tpl is used in "no header block" mode in this case www/templates/signup.tpl: + includes messages.tpl given that header.tpl is used in "no header block" mode in this case + smarty `if` blocks around uses of `$newmember.givenname` etc. and `$newmembernotes` www/templates/listusers.tpl: + smarty `if` blocks around uses of `$user.description` and `$user.type`
Thanks @unixnut! We've pulled the changes. |
than installing the (now defunct) php-auth package
(Still needs to go through User Acceptance Testing.)