DomainConfigUIManager strlen Passing null to parameter error

I looked at the code and decided to refactor it.

MR !106 main changes:

1. Methods getSelectedDomainId and getSelectedLanguageId strictly return NULL or a non-empty string. Both for the language and for the domain, an empty string is not a valid case.
2. Rewrote the code for getting data from a request or session to make it more readable, based on the current solution.
3. Added tests.

Great finding and awesome report @davps +1 and well explained. Since this seemed to be caused by the same changes beforehand like 🐛 DomainConfigOverrider is initializing every time, making it very slow Needs review from your investigations, which you have already commented on, I would like to ask you to comment here how these both issues possibly interfere committing both. Thanks for the efforts in here!

Comments:

+    if (!empty($request_domain_id)) {
+      return $request_domain_id;
     }

PHPStan doesn't like the empty() construct and I this fails. There is an explicit NULL set just before this so is_null() should work.

Same issue a little further down:

+    $session_domain_id = $_SESSION['domain_config_ui_domain'] ?? NULL;
+    if (!empty($session_domain_id)) {
+      return $session_domain_id;
     }

+    if (!empty($request_language_id)) {
+      return $request_language_id;
     }
-    if (is_null($id) && isset($_SESSION['domain_config_ui_language'])) {
-      $id = $_SESSION['domain_config_ui_language'];
+
+    $session_language_id = $_SESSION['domain_config_ui_language'] ?? NULL;
+    if (!empty($session_language_id)) {
+      return $session_language_id;
     }

Here, I am not a big fan of multiple return values, which can make code harder to maintain.

   /**
    * {@inheritdoc}
    */
   public function getSelectedLanguageId() {
-    $id = NULL;
-
     $request = $this->getRequest();
-    if (!is_null($request)) {
-      $id = $this->currentRequest->get('domain_config_ui_language') ?? NULL;
+    $request_language_id = !is_null($request)
+      ? $this->currentRequest->get('domain_config_ui_language')
+      : NULL;
+
+    if (!empty($request_language_id)) {
+      return $request_language_id;
     }
-    if (is_null($id) && isset($_SESSION['domain_config_ui_language'])) {
-      $id = $_SESSION['domain_config_ui_language'];
+
+    $session_language_id = $_SESSION['domain_config_ui_language'] ?? NULL;
+    if (!empty($session_language_id)) {
+      return $session_language_id;
     }
 
-    return $id;
+    return NULL;
   }

That said -- I LOVE THE TESTS.

The MR fixes the error for me. @davps, thanks for the fix!

Thanks for the report, but "fixes the error" for one person is not enough reason to RTBC an MR after 2 review comments not resolved/answered yet. Can somebody please answer/elaborate on the review comments raised? If it turns out to be finally OK or not an issue at all: great. @davps awesome work! +1

wrong status regarding unresolved/unanswered review questions.

Still needs review.

What is missing exactly to get this fixed?