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

Remove Mage_Sendfriend #4274

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

sreichel
Copy link
Contributor

@github-actions github-actions bot added documentation Template : rwd Relates to rwd template Template : base Relates to base template Component: Sendfriend Relates to Mage_Sendfriend translations Relates to app/locale XML Layout labels Oct 14, 2024
@kiatng
Copy link
Contributor

kiatng commented Oct 14, 2024

Orphaned references

in Mage_Adminhtml:

[
'value' => Mage_Sendfriend_Helper_Data::CHECK_IP,
'label' => Mage::helper('adminhtml')->__('IP Address')
],
[
'value' => Mage_Sendfriend_Helper_Data::CHECK_COOKIE,
'label' => Mage::helper('adminhtml')->__('Cookie (unsafe)')
],

email template:
https://github.com/OpenMage/magento-lts/blob/main/app/locale/en_US/template/email/product_share.html

ACL resource:

<sendfriend translate="title">
<title>Email to a Friend</title>
<sort_order>140</sort_order>
</sendfriend>

However, removing the above would break Mage_Sendfriend if install from composer.

@sreichel sreichel marked this pull request as draft October 14, 2024 06:10
@sreichel sreichel removed the request for review from colinmollenhour October 14, 2024 06:12
@kiatng
Copy link
Contributor

kiatng commented Oct 14, 2024

Alternative extension: https://github.com/pyrroman/social-sharing-button-for-magento

@sreichel
Copy link
Contributor Author

@kiatng Thanks. I have seen that sendfriend has some references in Mage_Reports observers, but i completly missed that.

@sreichel
Copy link
Contributor Author

sreichel commented Oct 14, 2024

Alternative extension: https://github.com/pyrroman/social-sharing-button-for-magento

Thanks. This will be mentioned in documention :)

@sreichel
Copy link
Contributor Author

There is also ...

class Mage_Catalog_Model_Resource_Eav_Mysql4_Sendfriend extends Mage_Sendfriend_Model_Resource_Sendfriend

@sreichel
Copy link
Contributor Author

Alternative extension: https://github.com/pyrroman/social-sharing-button-for-magento

Added to #4156

# Conflicts:
#	app/code/core/Mage/Sendfriend/etc/system.xml
@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Oct 14, 2024
@sreichel
Copy link
Contributor Author

However, removing the above would break Mage_Sendfriend if install from composer.

What is the problem with it?

Please test

ddev composer localdev https://github.com/OpenMage/module-mage-sendfriend
ddev composer require openmage/module-mage-sendfriend:dev-main

@github-actions github-actions bot added the Component: Catalog Relates to Mage_Catalog label Oct 14, 2024
@sreichel sreichel marked this pull request as ready for review October 14, 2024 22:18
kiatng
kiatng previously approved these changes Oct 15, 2024
ADDISON74
ADDISON74 previously approved these changes Oct 20, 2024
Copy link
Contributor

@ADDISON74 ADDISON74 left a comment

Choose a reason for hiding this comment

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

I hope the PR covers all records and nothing is left behind.

@sreichel
Copy link
Contributor Author

I can ensure there is code left behind. Observer from Mage_Reports eg ... but nothing that should rely on removed code.

kiatng and others added 2 commits October 22, 2024 13:30
# Conflicts:
#	app/design/frontend/base/default/template/sendfriend/send.phtml
#	app/design/frontend/rwd/default/template/sendfriend/send.phtml
@sreichel sreichel dismissed stale reviews from ADDISON74 and kiatng via eb475d9 October 31, 2024 19:10
colinmollenhour
colinmollenhour previously approved these changes Nov 4, 2024
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@ADDISON74
Copy link
Contributor

ADDISON74 commented Nov 4, 2024

@sreichel - Why these files still remain, having no content, instead of being deleted as well?

  • app/design/frontend/rwd/default/template/sendfriend/send.phtml
  • app/design/frontend/base/default/template/sendfriend/send.phtml

Both are mentioned in the layout file app/design/frontend/base/default/layout/sendfriend.xml which you deleted.

Also, there is a conflict to be solved related to the app/code/core/Mage/Sendfriend/etc/system.xml file.

# Conflicts:
#	app/code/core/Mage/Sendfriend/etc/system.xml
@sreichel
Copy link
Contributor Author

sreichel commented Nov 5, 2024

For some reasons they have been restored in eb475d9.

Removed.

@ADDISON74
Copy link
Contributor

ADDISON74 commented Nov 5, 2024

@sreichel - I checkout this branch and there are two directories left behind

/app/code/core/Mage/Sendfriend
/app/code/core/Mage/Sendfriend/sql

I reloaded from disk a few times to be sure. If I am wrong a double check is welcome.

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 Component: Catalog Relates to Mage_Catalog Component: Sendfriend Relates to Mage_Sendfriend documentation Template : base Relates to base template Template : rwd Relates to rwd template translations Relates to app/locale XML Layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants