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

Added methods to access different sessions #4362

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sreichel
Copy link
Contributor

Description (*)

...w/o have to type Mage::getSingleton('some/session') ...

@github-actions github-actions bot added Component: PayPal Relates to Mage_Paypal Component: Core Relates to Mage_Core Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: Reports Relates to Mage_Reports Component: CatalogInventory Relates to Mage_CatalogInventory Component: Checkout Relates to Mage_Checkout Component: AdminNotification Relates to Mage_AdminNotification Component: Sales Relates to Mage_Sales Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Component: Adminhtml Relates to Mage_Adminhtml Component: ProductAlert Relates to Mage_ProductAlert Component: Page Relates to Mage_Page Component: Api PageRelates to Mage_Api Component: Captcha Relates to Mage_Captcha Component: Contacts Relates to Mage_Contacts Component: Tag Relates to Mage_Tag Component: CurrencySymbol Relates to Mage_CurrencySymbol Component: CatalogRule Relates to Mage_CatalogRule Component: Admin Relates to Mage_Admin Component: SalesRule Relates to Mage_SalesRule labels Nov 17, 2024
@github-actions github-actions bot added Component: Index Relates to Mage_Index Component: Downloadable Relates to Mage_Downloadable Component: Bundle Relates to Mage_Bundle Component: CatalogIndex Relates to Mage_CatalogIndex Component: Api2 Relates to Mage_Api2 Component: Log Relates to Mage_Log Component: CatalogSearch Relates to Mage_CatalogSearch Component: Authorizenet Relates to Mage_Authorizenet Component: Centinel Relates to Mage_Centinel Component: Dataflow Relates to Mage_Dataflow Component: GoogleAnalytics Relates to Mage_GoogleAnalytics Component: Install Relates to Mage_Install Component: Uploader Relates to Mage_Uploader Component: Rss Relates to Mage_Rss Component: Paygate Relates to Mage_Paygate phpstan labels Nov 17, 2024
@fballiano
Copy link
Contributor

wouldn't it be be easier to just have Mage::getAdminSession() Mage::getCoreSession(), it seem it would have less impact

@justinbeaty
Copy link
Contributor

While I like @fballiano's idea more, I'm not sure I really see the need for this change at all? There are tons of other models called with Mage::getSingleton(), why make special method for these and not others? What if I make a custom session model, then there's an inconsistency between how I get session models.

@kiatng
Copy link
Contributor

kiatng commented Nov 18, 2024

With copilot, and especially cursor AI, more and more typing are done by them, hence less and lees need for this PR.

@sreichel
Copy link
Contributor Author

sreichel commented Nov 18, 2024

wouldn't it be be easier to just have Mage::getAdminSession() Mage::getCoreSession(), it seem it would have less impact

What do you mean?

Add helper methods for "core" and "admin" sessions only (and leave everything else at it is)?

There are tons of other models called with Mage::getSingleton(), why make special method for these and not others?

Absolutly aggree! E.g. there is Mage::getSingleton('eav/config') w/ ~ 100 occurences ...

Yes. This can also be "fixed/improved" by adding getEavConfig() or something like that, but that should not be part of that PR.

With copilot, and especially cursor AI, more and more typing are done by them, hence less and lees need for this PR.

I dont use any of that. Now ... in phpstorm i can use [$this->] + g + S ... for autocomplete get[some]Session()

@justinbeaty
Copy link
Contributor

justinbeaty commented Nov 18, 2024

There are tons of other models called with Mage::getSingleton(), why make special method for these and not others?

Absolutly aggree! E.g. there is Mage::getSingleton('eav/config') w/ ~ 100 occurences ...

Yes. This can also be "fixed/improved" by adding getEavConfig() or something like that, but that should not be part of that PR.

I don't understand though... The Mage::getSingleton() method caches instances in the registry so it's already fast. Your implementation just adds a wrapper around Mage::getSingleton().

Why then not also special methods for getting regular models too?

$ git grep "Mage::getModel('catalog/product')" | wc -l
147

@sreichel
Copy link
Contributor Author

Yes, Its only a wrapper. (that was the idea of it 😄)

Why then not also special methods for getting regular models too?

$ git grep "Mage::getModel('catalog/product')" | wc -l
147

+1 for getProductModel() or getCatalogProductModel(). In another PR?

@justinbeaty
Copy link
Contributor

Personally I am just not a fan... For one, it's adding references to different modules in Mage_Core. Also, where does it end? Which models are important enough and which ones aren't?

@sreichel sreichel marked this pull request as draft November 19, 2024 00:30
@sreichel
Copy link
Contributor Author

sreichel commented Dec 7, 2024

For one, it's adding references to different modules in Mage_Core.

True. But traits are extendable. We can still split it into modules. (?)

Also, where does it end?
Which models are important enough and which ones aren't?

I think i have covered ALL session types .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml Component: AdminNotification Relates to Mage_AdminNotification Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 Component: Authorizenet Relates to Mage_Authorizenet Component: Bundle Relates to Mage_Bundle Component: Captcha Relates to Mage_Captcha Component: Catalog Relates to Mage_Catalog Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogRule Relates to Mage_CatalogRule Component: CatalogSearch Relates to Mage_CatalogSearch Component: Centinel Relates to Mage_Centinel Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Contacts Relates to Mage_Contacts Component: Core Relates to Mage_Core Component: CurrencySymbol Relates to Mage_CurrencySymbol Component: Customer Relates to Mage_Customer Component: Dataflow Relates to Mage_Dataflow Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: GoogleAnalytics Relates to Mage_GoogleAnalytics Component: Index Relates to Mage_Index Component: Install Relates to Mage_Install Component: Log Relates to Mage_Log Component: Newsletter Relates to Mage_Newsletter Component: Oauth Relates to Mage_Oauth Component: Page Relates to Mage_Page Component: Paygate Relates to Mage_Paygate Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: ProductAlert Relates to Mage_ProductAlert Component: Rating Relates to Mage_Rating Component: Reports Relates to Mage_Reports Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Shipping Relates to Mage_Shipping Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Uploader Relates to Mage_Uploader Component: Weee Relates to Mage_Weee Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist phpstan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants