Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the login flow by migrating the legacy Zend-based login view/controller/form to ipl/compat equivalents and consolidating the login page markup into a reusable widget, with related CSS/JS adjustments.
Changes:
- Replace the Zend
LoginFormwith an iplCompatFormimplementation and adapt login handling to the compat lifecycle. - Replace the legacy
login.phtmlview script with a newLoginPagewidget used by aCompatController-basedAuthenticationController. - Update login-related CSS/JS selectors and styles, including spacing for additional login buttons and a typo fix in orb IDs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| public/js/icinga/history.js | Adjusts the login selector used during onload redirect handling for the new nesting. |
| public/css/icinga/login.less | Updates layout and spacing for the new login markup and added login buttons container. |
| public/css/icinga/login-orbs.less | Fixes the orb ID typo to match the updated markup (orb-notifications). |
| library/Icinga/Web/Widget/LoginPage.php | Introduces a widget encapsulating the full login page structure (form, footer, social links, orbs). |
| application/views/scripts/authentication/login.phtml | Removes the legacy view script in favor of the widget-based rendering. |
| application/forms/Authentication/LoginForm.php | Migrates the login form to CompatForm, adds CSRF/UID, and updates success/error handling. |
| application/controllers/AuthenticationController.php | Migrates controller to CompatController and renders LoginPage instead of a view script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'class' => $this->getPopulatedValue('username') === null ? 'autofocus' : '', | ||
| 'placeholder' => $this->translate('Username'), | ||
| 'decorators' => [ | ||
| 'RenderElement' => new RenderElementDecorator(), |
Contributor
Author
There was a problem hiding this comment.
I don't think the username field does need an errors decorator as it's never rendering an error.
| $setupNote = $this->translate( | ||
| 'It appears that you did not configure Icinga Web 2 yet so it\'s not possible to log in' | ||
| . ' without any defined authentication method. Please define an authentication method by' | ||
| . ' following the instructions in the %s or by using our %s.', |
Comment on lines
+180
to
+185
| 'a', | ||
| Attributes::create([ | ||
| 'href' => 'https://www.facebook.com/icinga', | ||
| 'target' => '_blank', | ||
| 'title' => $this->translate('Icinga on Facebook'), | ||
| 'aria-label' => $this->translate('Icinga on Facebook') |
Contributor
Author
There was a problem hiding this comment.
I don't think this is necessary, because we link to our own page.
Comment on lines
+198
to
+203
| 'a', | ||
| Attributes::create([ | ||
| 'href' => 'https://github.com/Icinga', | ||
| 'target' => '_blank', | ||
| 'title' => $this->translate('Icinga on GitHub'), | ||
| 'aria-label' => $this->translate('Icinga on GitHub'), |
Contributor
Author
There was a problem hiding this comment.
I don't think this is necessary, because we link to our own page.
| .errors { | ||
| background-color: @color-critical; | ||
| color: white; | ||
| margin: 0; |
Contributor
Author
There was a problem hiding this comment.
This is already done by form.icinga-forms .errors in forms.less.
Replace the legacy Zend-based `LoginForm` with an ipl `CompatForm`-based form:
- Add `CsrfCounterMeasure` and `FormUid` traits
- Replace `init()`/`createElements()` with `__construct()´/`assemble()` using
inline ipl decorator objects instead of Zend decorator arrays
- Replace `getRedirectUrl()` with `createRedirectUrl()` because
`getRedirectUrl()` would override `ipl\Html\Form::getRedirectUrl()`
- use `hasBeenAssembled()` instead of `$this->created`
- use `str_contains()` instead of `strpos()`
- Rewrite `onSuccess()` for `CompatForm`:
- void return type
- use `Icinga::app()->getResponse()` instead of `$this->getResponse()`
- use `addMessage()` instead of `addError()`
- call `setRedirectUrl()` explicitly
- Expose `onError()` as public so controller event listeners can call it
Extract the login page structure (logo, footer, social links, decorative orbs) from `login.phtml` into a reusable ipl-html widget. Extending `HtmlDocument` rather than `BaseHtmlElement` lets it emit `#login` and the seven `.orb` sibling divs without a wrapping root tag. Both the login action and the upcoming 2FA challenge action can now share the same visual scaffolding without duplicating markup and without using view scripts anymore.
Convert `AuthenticationController` from the legacy Zend Controller to `CompatController`, dropping `login.phtml` in favour of the new `LoginPage` widget and `addContent()`. The view variable assignments are replaced by `setTitle()` and `addContent(new LoginPage(...))`. In `CompatController`, `$this->controls` is the tab bar area rendered above the page content. When no tabs are added it still emits an empty `<div class="controls">` wrapper. Replacing it with a bare `HtmlDocument` which renders as nothing when empty suppresses that wrapper entirely, keeping the login page markup clean. Handle the redirect on success in an `ON_SUBMIT` event handler. Delegate the external-backend-only check to `LoginForm::onRequest()` via `ON_REQUEST`. Use `$this->getServerRequest()` instead of `ServerRequest::fromGlobals()`. Two structural fixes required by the changed DOM nesting: - `login.less`: height `100%` -> `100vh` (`#login` is now inside `.content` which has no explicit height, so percentage inheritance breaks) - `history.js`: `#layout > #login` -> `#layout #login` (direct-child selector breaks because `#login` is now a grandchild of `#layout` through `.content`)
Adjust login.less to match the HTML structure emitted by CompatForm: - Add width: unset and margin on `.icinga-form` inside `.login-form` so the ipl form does not overflow its container and control groups have spacing - Restyle `.errors` at the li level rather than the container level, giving each error message its own red box with margin between them - Remove the padding from `.errors`/`.form-errors` that was set at the container level (now on each li) - Change `.remember-me-box` align-items from `flex-start` to `center` so the toggle switch and label sit at the same baseline Also fix a pre-existing typo in `login-orbs.less`: `#orb-notifactions` -> `#orb-notifications`
Group the `LoginButtonHook`-provided buttons inside a `div.login-buttons` so they can be styled independently from the main form submit button, and skip the wrapper entirely when no additional buttons are present.
Ensure `.errors` inside `.login-buttons` has no extra padding and that the last error item retains a bottom margin, matching the layout used for the primary login form errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR replaces the legacy Zend-based login form and authentication controller with their ipl equivalents, and extracts the login page view script markup into a reusable widget.
Convert
LoginFormtoCompatFormLoginFormnow extendsCompatForm, replacinginit()/createElements()with__construct()/assemble()and Zend decorator arrays with inline ipl decorator objects. TheCsrfCounterMeasureandFormUidtraits are added.getRedirectUrl()is renamed tocreateRedirectUrl()to avoid overridingCompatForm::getRedirectUrl(), usinghasBeenAssembledinstead of$this->createdandstr_contains()instead ofstrpos().onSuccess()is rewritten for the CompatForm lifecycle: void return, explicitsetRedirectUrl()call,Icinga::app()->getResponse()instead of$this->getResponse(), andaddMessage()instead ofaddError().New
LoginPagewidget to replace view scriptExtracts the login page structure (logo, footer, social links, decorative orbs) from
login.phtmlinto a newLoginPagewidget extendingHtmlDocument, so the markup is defined in one place and no longer lives in a view script. Additional login buttons provided byLoginButtonHookare now grouped in adiv.login-buttonsflex container, rendered only when buttons are actually present.Convert
AuthenticationControllertoCompatControllerAuthenticationControllernow extendsCompatController, droppinglogin.phtmlin favour ofLoginPageandaddContent(). The form is built with anON_SUBMIThandler for the redirect and anON_REQUESThandler delegating toLoginForm::onRequest(). Uses$this->getServerRequest()instead ofServerRequest::fromGlobals().CSS / JS fixes
login.less:height: 100%->100vhon#login(now a grandchild of#layoutvia.content).login-buttonsstyled as a flex column with a row gap and top marginlierror styling, including spacing inside.login-buttons.icinga-formwidth and control-group spacingalign-items: centeron.remember-me-boxmargin-rightreset for the disabled-state descriptionhistory.js:#layout > #login->#layout #login(direct-child selector broke with the new nesting)login-orbs.less: fixes long-standing typo#orb-notifactions->#orb-notificationsresolve #5495