Fixed warning Undefined array key 0 when installing OM via command line (dev mode on) (#3672)

[akunzai]

Description (*)

Handle posible false value in App::getWebsite from Store::getWebsite at mysql4-data-upgrade-1.4.0.0.7-1.4.0.0.8.php

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Undefined array key 0 warning at console Installation #3672

Manual testing scenarios (*)

1. create docker env from this gist
2. run the containers
3. change the source code in OpenMage container as this PR
4. run the install.sh from gist to install OpenMage

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[sreichel]

Not sure if its the best way, but i have no better idea right now. Needs some investigation.

[fballiano]

fine to me

[sreichel]

false leads to another exception in behind.

I run into same and did not fix it that way. At least add a "todo" annotation.

[Sekiphp]

This can provide unexpectable behavior of application. In ideal case method should throw Exception, that website not exists and fix places, where the error is provided.

I do not think, that if you call Mage::app()->getWebsite(false) you should get current website instance.

[midlan]

Why not just fix the file mysql4-data-upgrade-1.4.0.0.7-1.4.0.0.8.php?

[akunzai]

Thanks for everyone's advice. I made some changes to this PR, and I think that null should be enough to throw exceptions and clarify that the website does not exist!

[midlan]

@akunzai I still believe we shouldn't modify one of the most commonly used method in code. Instead, let's focus on fixing the specific file where you discovered the issue.

[midlan]

current solution is ok for me, approving

[fballiano]

@akunzai silly question, did you test the current implementation of this PR?

[akunzai]

@fballiano Yes, I tested the CLI installation via with current implementation.

[sreichel]

It "works", but seems wrong as @Sekiphp pointed out.

[akunzai]

Please explain your point, I didn't get it. or fire another PR to fix it?

[fballiano]

@akunzai is the latest implementation of this PR tested?

everybody: Since this PR patches the install file, it seems to be ok to me and I'd merge it, anything against it?

[sreichel]

I do not think, that if you call Mage::app()->getWebsite(false) you should get current website instance.

This. But i had no better idea so far.

[fballiano]

I do not think, that if you call Mage::app()->getWebsite(false) you should get current website instance.

This. But i had no better idea so far.

@sreichel in that case it could be fixed in a different place but could probably be another PR right?

[sreichel]

I really dont know ... as said, i faced same issue and thought about to fix it that way, but it leads to another exception in background (needs to turn dev-mode on, or xdebug on ... )

imho ... this need more investigation

[akunzai]

@fballiano

The console installation success without any errors with the latest implementation of this PR.

[sreichel]

@akunzai this error only occurs with dev-mode on.

You can set the environment variable to 0 in CLI too.

This "fix" seems wrong to me. (otherwise I'd have already made a pr)

[akunzai]

@sreichel

1. In my setup, I have already turn on the dev-mode by set MAGE_IS_DEVELOPER_MODE=1, Are we using the same environment?
2. Can you provide the error details (Include the file and line number information)?

[fballiano]

@akunzai I'm testing this PR now but I get this error:

ERROR: Error in file: "openmage/app/code/core/Mage/Sales/sql/sales_setup/upgrade-1.6.0.3-1.6.0.4.php" - Deprecated functionality: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /Users/fab/Projects/openmage/lib/Varien/Db/Adapter/Pdo/Mysql.php on line 1783

[akunzai]

@fballiano

In my setup, the deprecated errors was suppressed by ~E_STRICT & ~E_DEPRECATED,

IMHO, the deprecated errors should be fix in another PRs.

[fballiano]

I don't think it's that useful without fixing #3677 (comment) (and probably more that would appear) but ok