Skip to content

Conversation

@grandeljay
Copy link
Contributor

Due to my file structure App::getShopRoot returns the wrong directory.

  • modified-shop directory: [...]/grandeljay/modified-shop (contains a symbolic link to MMLC directory)
  • MMLC directory: [...]/RobinTheHood/ModifiedModuleLoaderClient

In this setup App:getShopRoot returns realpath(__DIR__ . '/../../../') which in my case is [...]/RobinTheHood instead of [...]/grandeljay/modified-shop.

I think it would be best to allow an override to this behaviour.

Jay and others added 30 commits August 19, 2020 11:48
- fix: headings not looking good with module icon
- fix: module images being too long
- fix: remove unnecessary html containers
- fix: remove inline css
- fix: change loading text to german equivelant
- style: change the text cursor of badges to default cursor
- style: cleaned up some unnecessary code
@grandeljay
Copy link
Contributor Author

Psalm is saying

ERROR: PossiblyNullArgument - src/Classes/App.php:45:27 - Argument 1 of rtrim cannot be null, possibly null value provided (see https://psalm.dev/078)
                  : rtrim(Config::getOption('shopRoot'), '/\\');

but empty() returns true if getOption returns null

Returns true if var does not exist or has a value that is empty or equal to zero, aka falsey, see conversion to boolean. Otherwise returns false.

https://www.php.net/manual/en/function.empty.php

So I believe Psalm has triggered a false positive and this static function is fine.

@RobinTheHood
Copy link
Owner

Psalm is saying

ERROR: PossiblyNullArgument - src/Classes/App.php:45:27 - Argument 1 of rtrim cannot be null, possibly null value provided (see https://psalm.dev/078)
                  : rtrim(Config::getOption('shopRoot'), '/\\');

but empty() returns true if getOption returns null

Returns true if var does not exist or has a value that is empty or equal to zero, aka falsey, see conversion to boolean. Otherwise returns false.
https://www.php.net/manual/en/function.empty.php

So I believe Psalm has triggered a false positive and this static function is fine.

But Config::getOption() dose:

return !empty($configuration[$option]) ? $configuration[$option] : null; // Null ist possible

and not

return !empty($configuration[$option]);

@grandeljay
Copy link
Contributor Author

That's no problem since null is falsey. So even in this case:

public static function getShopRoot(): string
{
    $shopRoot = empty(null)
              ? realpath(__DIR__ . '/../../../')
              : rtrim(Config::getOption('shopRoot'), '/\\');

    return $shopRoot;
}

rtrim() can never be null since empty(null) evaluates as true.

@grandeljay grandeljay changed the title Allow defining the shop root Fix App::getShopRoot Jan 12, 2023
@RobinTheHood
Copy link
Owner

RobinTheHood commented Jan 12, 2023

I have seen you commit Automatically determine the shop root, but I thing I prefer Refactor, because there are situations where the mmlc is invoked by a script and global $_SERVER[] array is only available, if the mmlc is invoked by the browser/webserver. Maybe you can add a GUI option for this setting in /src/Templates/Settings.tmpl.php

@grandeljay grandeljay changed the title Fix App::getShopRoot Add option to define shop root Jan 13, 2023
@grandeljay
Copy link
Contributor Author

Some things I've noticed:

  • there was a problem with the logging (log directory was missing) so I added a commit for this too (sorry for mixing things)
  • writing the configuration always returns a green success message, even if it fails (i. e. because the config.php is read-only)

@RobinTheHood RobinTheHood merged commit a47ea57 into RobinTheHood:master Jan 13, 2023
@RobinTheHood RobinTheHood added this to the v1.20.0 milestone Mar 5, 2023
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