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

clean up logged error messages #258

Merged
merged 5 commits into from
Jul 18, 2024
Merged

clean up logged error messages #258

merged 5 commits into from
Jul 18, 2024

Conversation

briskt
Copy link
Contributor

@briskt briskt commented Jul 17, 2024

Fixed

  • Added a null coalesce on $state['mfaLearnMoreUrl'] for the review page
    • fixes error: SimpleSAML\Error\Exception: Warning - Undefined array key "mfaLearnMoreUrl" at /data/vendor/simplesamlphp/simplesamlphp/modules/profilereview/public/nag.php:37
  • Defined exampleauth:UserPass users in users key
    • fixes error: Module exampleauth:UserPass configured in legacy mode. Please put your username:password entries under the "users" key in your authsource.
  • Changed from if to assert to catch error condition earlier
  • Stop using the admin module to access the hub discovery page
    • fixes error: Caused by: Twig\Error\RuntimeError: An exception has been thrown during the rendering of a template ("SimpleSAML\XHTML\Template::getEntityDisplayName(): Argument Move handling of LOGENTRIES_KEY into separate shell script. #1 ($data) must be of type array, null given, called in /data/vendor/twig/twig/src/Environment.php(392) : eval()'d code on line 82").
  • Filled in missing data in authsources.php
    • fixes error: SimpleSAML\Error\Exception: Warning - Undefined array key "add" at /data/vendor/simplesamlphp/simplesamlphp/modules/profilereview/src/Auth/Process/ProfileReview.php:234

briskt added 5 commits July 17, 2024 12:42
fixes error shown in test log output:
SimpleSAML\Error\Exception: Warning - Undefined array key "add" at /data/vendor/simplesamlphp/simplesamlphp/modules/profilereview/src/Auth/Process/ProfileReview.php:234
fixes error shown in test log output:
Caused by: Twig\Error\RuntimeError: An exception has been thrown during the rendering of a template ("SimpleSAML\XHTML\Template::getEntityDisplayName(): Argument #1 ($data) must be of type array, null given, called in /data/vendor/twig/twig/src/Environment.php(392) : eval()'d code on line 82").
and remove incorrect log message
fixes error shown in test log output:
Module exampleauth:UserPass configured in legacy mode. Please put your username:password entries under the "users" key in your authsource.
fixes error:
impleSAML\Error\Exception: Warning - Undefined array key "mfaLearnMoreUrl" at /data/vendor/simplesamlphp/simplesamlphp/modules/profilereview/public/nag.php:37
Copy link

Copy link
Contributor

@hobbitronics hobbitronics left a comment

Choose a reason for hiding this comment

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

Looks good to me

@briskt briskt merged commit bfb7cf6 into develop Jul 18, 2024
3 checks passed
@briskt briskt deleted the feature/fix-errors branch July 18, 2024 15:11
$spmd = $state['SPMetadata'];
$this->log('Updated SP metadata from ' . $this->spEntityId . ' to ' . $spmd['entityid']);
}
assert($state && array_key_exists('SPMetadata', $state));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to depend on PHP's built in assert() function, since it is typically only run in debug or test mode, and is disabled in production mode (per the docs).

If we want to ensure something is asserted every time, it seems to be better to use other/normal PHP code (either an if and throw new Exception, or something like WebMozart's "Assert" library).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize that. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants