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

Fixed a couple of null parameter warnings in the ImportExport module #3818

Closed
wants to merge 2 commits into from
Closed

Fixed a couple of null parameter warnings in the ImportExport module #3818

wants to merge 2 commits into from

Conversation

fballiano
Copy link
Contributor

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Feb 9, 2024
@fballiano fballiano changed the title Fixed a couple of null parameter warning in the ImportExport module Fixed a couple of null parameter warnings in the ImportExport module Feb 9, 2024
Comment on lines 95 to 99
if ($value === null) {
return '';
}

return $this->escapeHtml(strlen($value) > 0 ? $value : $default);
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot assume $default is always an empty string.

Suggested change
if ($value === null) {
return '';
}
return $this->escapeHtml(strlen($value) > 0 ? $value : $default);
return $value !== null && $this->escapeHtml(strlen($value) > 0 ? $value : $default);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kiatng true, I also did it in a previous PR but I didn't notice here :-D

I did it in a different way (more similar to the other one, for readability), what do you think?

thanks!!

Comment on lines +110 to 112
// Set entered data if was error when we do save
$profile = Mage::registry('current_convert_profile');

// set entered data if was error when we do save
$data = Mage::getSingleton('adminhtml/session')->getConvertProfileData(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should be above $data as it is related to it:

Suggested change
// Set entered data if was error when we do save
$profile = Mage::registry('current_convert_profile');
// set entered data if was error when we do save
$data = Mage::getSingleton('adminhtml/session')->getConvertProfileData(true);
/** @var Mage_Dataflow_Model_Profile $profile */
$profile = Mage::registry('current_convert_profile');
// Repopulate user input values in the edit form in case of saving error
$data = Mage::getSingleton('adminhtml/session')->getConvertProfileData(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reformatted it this way because it seems that comment belongs to this whole logical section:
Screenshot 2024-02-09 alle 09 56 10

I can revert it but those variables and that comment kinda belong together :-)

@kiatng
Copy link
Contributor

kiatng commented Feb 9, 2024

I think #3811 is unrelated to null deprecation and should be a separate PR should it be a bug. At this point, I am not sure it is.

@fballiano
Copy link
Contributor Author

I think #3811 is unrelated to null deprecation and should be a separate PR should it be a bug. At this point, I am not sure it is.

I just didn't want to create too many PRs in one go so I thought about merging at least the two that seemed a bit related to each other :-\

@addison74
Copy link
Contributor

I just tested this PR.

Case 1
In the Advanced Profiles grid page, press the [Add New Profile] button. Then fill in "Profile Name" and "Actions XML" with some data. As soon as you press the [Save] button, you get the following error

Fatal error:  Uncaught Error: Call to a member function getId() on null in /var/www/html/app/code/core/Mage/Adminhtml/controllers/System/Convert/ProfileController.php:117
Stack trace:
#0 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Action.php(421): Mage_Adminhtml_System_Convert_ProfileController->editAction()
#1 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(255): Mage_Core_Controller_Varien_Action->dispatch()
#2 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Front.php(181): Mage_Core_Controller_Varien_Router_Standard->match()
#3 /var/www/html/app/code/core/Mage/Core/Model/App.php(358): Mage_Core_Controller_Varien_Front->dispatch()
#4 /var/www/html/app/Mage.php(760): Mage_Core_Model_App->run()
#5 /var/www/html/index.php(56): Mage::run()
#6 {main}
  thrown in /var/www/html/app/code/core/Mage/Adminhtml/controllers/System/Convert/ProfileController.php on line 117

Case 2
In my case there is already a profile in the grid and I will edit it. In the editing page, press any of the [Save Profile] or [Save and Continue Edit] buttons and the following message is displayed

screenshot

There are two files affected if this PR is merged.

@addison74
Copy link
Contributor

This PR must be split into two because it does not solve any of the reported problems.

In the case of the Profiles grid. Press the [Add New Profiles] button. Fill in "Name" and press the [Save and Continue Edit] button. The following error appears at the top

screenshot

@fballiano fballiano closed this Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml
Projects
None yet
3 participants