Skip to content

4240: Added handler and element usage overview module #418

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

jekuaitk
Copy link
Contributor

@jekuaitk jekuaitk commented May 6, 2025

https://leantime.itkdev.dk/#/tickets/showTicket/4240

  • Adds handler and element usage overview page (internally referred to as "Gammelt skrammel")

@jekuaitk jekuaitk requested a review from rimi-itk May 12, 2025 12:48
Copy link
Contributor

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some of the best code I've ever seen. It's obviously written by a real intelligence. Some minor cleanups suggested.

Comment on lines 25 to 26
readonly private WebformElementManagerInterface $webformElementManager,
readonly private WebformEntityStorageInterface $webformEntityStorage,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think

Suggested change
readonly private WebformElementManagerInterface $webformElementManager,
readonly private WebformEntityStorageInterface $webformEntityStorage,
private readonly WebformElementManagerInterface $webformElementManager,
private readonly WebformEntityStorageInterface $webformEntityStorage,

reads nicer …

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

* @phpstan-return array<string, mixed>
*/
public function buildForm(array $form, FormStateInterface $form_state) {
$form_state->setMethod('GET');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$form_state->setMethod('GET');
$form_state->setMethod(Request::METHOD_GET);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Comment on lines 55 to 57
$form['#cache'] = [
'max-age' => 0,
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this form is only shown to authenticated users, and that this therefork works (cf. https://www.drupal.org/docs/drupal-apis/cache-api/cache-max-age#s-limitations-of-max-age). But it may be better to use no_cache (https://www.drupal.org/docs/drupal-apis/routing-system/structure-of-routes) in ….routing.yml to disable caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to no_cache.

Comment on lines 26 to 27
readonly private WebformHandlerManagerInterface $webformHandlerManager,
readonly private WebformEntityStorageInterface $webformEntityStorage,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonly private WebformHandlerManagerInterface $webformHandlerManager,
readonly private WebformEntityStorageInterface $webformEntityStorage,
private readonly WebformHandlerManagerInterface $webformHandlerManager,
private readonly WebformEntityStorageInterface $webformEntityStorage,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Comment on lines 55 to 58
$form_state->setMethod('GET');
$form['#cache'] = [
'max-age' => 0,
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on caching elsewhere …

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to no_cache.

@jekuaitk jekuaitk requested a review from rimi-itk May 13, 2025 06:53
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