search

mage-os-lab / web-installer

4 stars 4 forks source link

Constructor property promotion #8

Open rhoerr opened 22 hours ago

rhoerr commented 22 hours ago

Reviewing code: src/Magento/Setup/Controller/Environment.php uses constructor property promotion. I didn't notice any other instances. Is this fine, or best avoided still?

I haven't tried it, but I wonder if this (in and of itself?) would prevent a friendly error page if you try to install on PHP 7.4.

    public function __construct(
        protected \Magento\Framework\Setup\FilePermissions $permissions,
        protected \Magento\Framework\Filesystem $filesystem,
        protected \Magento\Setup\Model\CronScriptReadinessCheck $cronScriptReadinessCheck,
        protected \Magento\Setup\Model\PhpReadinessCheck $phpReadinessCheck
    ) {
    }
jakwinkler commented 16 hours ago

Latest Magento requires PHP 8.1, I can downgrade this to PHP7.4 Imho, PHP8.1 should be the minimum requirement

rhoerr commented 10 hours ago

Overall, Mage-OS' current minimum PHP version is 8.1, same as Magento 2.4.7. But if someone tries running it on 7.4, what happens? And does this code impact that, or is it the same either way?

If it just shows a message 'PHP 8.1+ required' (which has been my experience in the past), unrelated to the installer, then we should just allow this usage, close the issue, and move on.

If it shows an error due to this code, I think we should make the code backwards compatible a bit more and let the compatibility checker handle it.