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

use of registry in Magento\Tax\Helper\Data #445

Closed
fooman opened this issue Dec 21, 2013 · 6 comments
Closed

use of registry in Magento\Tax\Helper\Data #445

fooman opened this issue Dec 21, 2013 · 6 comments
Assignees

Comments

@fooman
Copy link
Contributor

fooman commented Dec 21, 2013

This method here

https://github.com/magento/magento2/blob/master/app/code/Magento/Tax/Helper/Data.php#L937

should be refactored to work without the registry.

@verklov
Copy link
Contributor

verklov commented Dec 23, 2013

Hello fooman,
Thank you for reporting this issue!
Out team will investigate it and we will respond once we have the results.

@verklov
Copy link
Contributor

verklov commented Dec 26, 2013

Hello fooman,
Why do you think we should not use registry in this case? It is unclear for us at this point which problem will be solved by the suggested refactoring.

@fooman
Copy link
Contributor Author

fooman commented Dec 27, 2013

Using the registry in this way will make it very hard (and surprising) for any one else to use the method

public function getShippingTax($source)
{
    if ($this->_coreRegistry->registry('current_invoice')) {
        $current = $this->_coreRegistry->registry('current_invoice');
    } elseif ($this->_coreRegistry->registry('current_creditmemo')) {
        $current = $this->_coreRegistry->registry('current_creditmemo');
    } else {
        $current = $source;
    }

    $taxClassAmount = array();
    if ($current && $source) {
        if ($current->getShippingTaxAmount() != 0 && $current->getBaseShippingTaxAmount() != 0) {

For example let's say I want to add to the invoice confirmation email the following

Order Shipping Tax: XXX of which has been invoiced in this invoice: XXX

You would think that using getShippingTax($order) and getShippingTax($invoice) should work. However due to the registry use there is no way to get $current to be anything but the current_invoice. When you have multiple invoices per order the results would be incorrect.

@benmarks
Copy link
Contributor

👍

@wshqq
Copy link

wshqq commented Jan 4, 2014

Thumbs up. The function getShippingTax and getCalculatedTaxes didn't need the registry. We need ths function to get calculated Taxes for invoice or creditmemo, so we should pass those objects and not the order. From invoice and creditmemo you can call getOrder(). This function was added with Magento 1.6 and we have to register invoices and creditmemos every time, we want to use this function. This doesn't make sense.
This function is important for 3rd party extension dealing with invoices and creditmemos.

magento-team added a commit that referenced this issue Sep 26, 2014
* Various improvements:
   * Implemented a general way of using RSS module
   * Created a cron job in the Customer module for cleaning the customer_visitor table
   * Added a warning message to the Use HTTP Only option in the Admin panel
   * Implemented the Grid component in the Magento UI Library
   * Reimplemented the URL Rewrites functionality in the new UrlRedirect module
 * Framework improvements:
   * Added the ability to install Magento 2 using CLI
   * Aggregated Magento installation and upgrade into one tool
   * Refactored CustomerService REST WebApi to be more RESTful
   * Increased unit and integration test coverage
   * Moved page asset management to page configuration API, and eliminated the \Magento\Theme\Block\Html\Head block
   * Eliminated the Root, Html and Title blocks
 * Themes update:
   * Removed widgets from the default Magento installation
 * Fixed bugs:
   * Fixed an issue with wishlist creation for non-registered customer
   * Fixed an issue with Google Mapping where Condition did not show correct value
   * Fixed an issue  where there were too many notifications for admin user by default
   * Fixed a Daylight Savings Time calculation error
   * Fixed an issue where default cookie path and lifetime were not validated prior to saving
   * Fixed an issue where current admin password was not required for resetting admin password
   * Fixed an issue where custom customer attribute or customer address attribute was not accessible when custom_attribute is used as the attribute code
   * Fixed an issue where integration entity could not be deleted after being searched in grid
   * Fixed an issue where invalid parameter value was shown in SOAP
   * Fixed an issue where exception was thrown for Array to String conversion in SOAP
   * Fixed an issue where exception was thrown due to invalid argument supplied for foreach() statement in REST
   * Fixed an issue where admin tax notifications did not appear correctly in the System Messages dialog box
   * Fixed an issue where tax details were missing when viewing order in the Admin panel
   * Fixed an issue where styles for the storefront store selector were absent
   * Fixed an issue where customer got 404 page when switching store views on the product page of a product with different URL keys in different store views
   * Fixed an issue where the Add To Cart button in the MAP pop-up did not work for configurable and bundle products
   * Fixed an issue where for specifying options for configurable product was absent after adding a product from the MAP pop-up
   * Fixed an issue where a fatal error was thrown after selecting shipping method on PayPal Express Checkout
   * Fixed an issue with sending invoice email
   * Fixed an issue where integration tests failed with a fatal error
   * Fixed an issue where credit memo entry was not created after performing a refund for an order
   * Fixed an issue where categories layout for widgets did not work
   * Fixed an issue where opening a page restricted by ACL lead to blank page instead of the Access Denied page
   * Fixed an issue where a blank page was displayed instead of the using the Advanced Search result
   * Fixed an issue where the "Please wait" spinner was absent on Ajax requests for order creation in the Admin panel
   * Fixed an issue with the main navigation menu location on the page
 * Modularity:
   * Implemented the automatic applying of the MAP policy
 * Indexers:
   * Eliminated the old Magento_Index module
 * Search library
   * Added wildcards filter
   * Eliminated unused queries and filters
   * Added IN to Term filter
   * Moved the "value" attribute from <match> to <query> for the Match query
   * Refactored the usage of negation
   * Implemented Request Builder
 * CatalogSearch adapter
   * Pluginized adding attribute to search index
   * Merged base declaration with searchable attributes
 * Added the following Setup CLI tools in the setup folder
   * Deployment Configuration Tool
   * Schema Setup and Update Tool
   * DB Data Update Tool
   * Admin User Setup Tool
   * User Configuration Tool
   * Installation Tool
   * Update Tool
 * GitHub requests:
   * [#615] (#615) -- Use info as object in checkout_cart_update_items_before
   * [#659] (#659) -- Recently viewed products sidebar issue
   * [#660] (#660) -- RSS global setting
   * [#663] (#663) -- session.save_path not valid
   * [#445] (#445) -- use of registry in Magento\Tax\Helper\Data
   * [#646] (#646) -- Fixed flat category indexer bug
   * [#643] (#643) -- Configurable Products Performance
   * [#640] (#640) -- [Insight] Files should not be executable
   * [#667] (#667) -- Tiny improvement on render() method in Column/Renderer/Concat
   * [#288] (#288) -- Add Cell Phone to Customer Address Form
   * [#607] (#607) -- sitemap.xml filename is not variable
   * [#633] (#633) -- Fixed Typo ($_attribite -> $_attribute)
   * [#634] (#634) -- README.md contains broken link to X.commerce Agreement
   * [#569] (#569) -- ObjectManager's Factory should be replaceable depending on service
   * [#654] (#654) -- Demo notice overlapping
 * Functional tests:
   * Abandoned carts report
   * Adding products from wishlist to cart
   * Create invoice for offline payment methods
   * Delete products from shopping cart
   * Delete widget
   * Global search
   * Order count report
   * Order total report
@verklov
Copy link
Contributor

verklov commented Sep 26, 2014

@fooman, the team resolved the issue that you reported. The code with the fix is now available in the repository. Thank you again for your contribution! We are closing this ticket now.

@verklov verklov closed this as completed Sep 26, 2014
magento-team pushed a commit that referenced this issue Jul 15, 2015
[API] Sprint 52 - Bugs Fixes
mmansoor-magento pushed a commit that referenced this issue Sep 29, 2016
Fixed issues:
 - MAGETWO-57868: Search fails with an error when used user-defined price attribute as searchable - for mainline
 - MAGETWO-57924: Unable to add "Customer Group Price"
 - MAGETWO-55341: Request to ESI doesn't return any data
 - MAGETWO-57892: L4 Plan fails on mainline branch
 - MAGETWO-56150: Order comments history shows time of comment twice
 - MAGETWO-57933: Product quantity is not updated on 'cart/configure' page second time
 - MAGETWO-53041: 'Yes/No' attribute shown in Advanced Search result even it not used in filtration
 - MAGETWO-52926: Impossible to find product on Store Front with name like lowerCamelCaseNumber
magento-engcom-team added a commit to okorshenko/magento2 that referenced this issue May 10, 2019
…o#445

 - Merge Pull Request magento/graphql-ce#445 from yogeshsuhagiya/graphql-ce:2.3-develop-graphql-PR-yogesh-2
 - Merged commits:
   1. ad4efb8
   2. 57d8908
   3. 8b39ecc
   4. 273ea48
   5. 43ba611
   6. 96c1e2c
   7. ec2ee49
magento-devops-reposync-svc pushed a commit that referenced this issue May 24, 2022
CABPI-389 Switch environment to production
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants