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.

Sign up for GitHub

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

Jump to bottom

\Magento\Framework\Pricing\PriceCurrencyInterface depends on Magento application code #682

Closed
joshdifabio opened this issue Oct 6, 2014 · 5 comments
Closed

\Magento\Framework\Pricing\PriceCurrencyInterface depends on Magento application code #682

joshdifabio opened this issue Oct 6, 2014 · 5 comments
Assignees
@vpelipenko
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@joshdifabio
Copy link
Contributor

Currently, \Magento\Framework\Pricing\PriceCurrencyInterface depends on:

  • \Magento\Store\Model\Store
  • \Magento\Directory\Model\Currency

Is it intended for PriceCurrencyInterface, which is part of the framework, to depend on application classes?

Furthermore, shouldn't those dependencies be on interfaces instead of on concrete implementations?

The functionality provided by this interface is very much coupled to Magento's application code (it's only real purpose seems to be to simplify working with a Store object, such that the consumer doesn't have to extract configuration from the Store object); as such it seems wrong for the interface to live in \Magento\Framework.

Apologies if this has already been covered.
@maksek
Copy link
Contributor

maksek commented Oct 21, 2014

Hi @joshdifabio, thanks for taking time and looking into code. We are going to reduce dependencies in further milestones. Moreover we are going to provide full set of interfaces for modules, see upcoming updates.

@maksek maksek self-assigned this Oct 21, 2014
@alankent
Copy link

Just to add to your question of "is this wrong"? You are absolutely correct. Magento\Framework should not depend on modules directly. Interfaces are fine with modules implementing the interfaces, but not modules directly.

@maksek maksek added the CS label Nov 12, 2014
@mcspronko
Copy link
Contributor

Hi @joshdifabio, we are currently working on the issue. Fix will be delivered in upcoming update.

@vpelipenko vpelipenko assigned vpelipenko and unassigned maksek Jan 9, 2015
@vpelipenko
Copy link
Contributor

Ticket: MAGETWO-30415. It is fixed and will be delivered in one of the next updates.

@vpelipenko vpelipenko added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Jan 21, 2015
tang-yu pushed a commit that referenced this issue Jan 23, 2015
MAGETWO-30415: [GITHUB] \Magento\Framework\Pricing\PriceCurrencyInter…
819015c 
…face depends on Magento application code #682
tang-yu pushed a commit that referenced this issue Jan 23, 2015
MAGETWO-30415: [GITHUB] \Magento\Framework\Pricing\PriceCurrencyInter…
b7cde02 
…face depends on Magento application code #682

    - Change parameter name in implementation class to be consistent with interface
@vpelipenko
Copy link
Contributor

This issue is fixed and available in 0.42.0-beta5.

magento-team added a commit to abeeskau/magento2-community-edition that referenced this issue Feb 9, 2015
@magento-team
0.42.0-beta5
868391e 
* UI improvements:
    * Updated the design of Enable Cookies CMS page
    * Implemented UI improvements for Widgets
    * Fixed the "Help Us to Keep Magento Healthy Report All Bugs (ver. #)" link Magento Admin
    * Various UI improvements
* Various improvements:
    * Implemented Sales Quote as a standalone Magento module
    * Performed custom EAV entities code and DB tables cleanup
    * Eliminating remnants of the Core module:
        * Moved Application Emulation from the Magento_Core module to the Magento_Store module
        * Moved Validator Factory from the Magento_Core module to the Magento Framework
    * Added static integrity test for composer.json files
    * Added PHPMD and PHPCS annotations to the codebase
* Tests improvements:
    * Added MVP tag to the functional tests
    * Created acceptance functional test suite
    * Replaced end-to-end tests for url rewrite creation, CMS page creation, category creation, review creation, customer frontend creation, and tax rule creation with injectable tests
    * Automated test cases for downloadable products with taxes
* Fixed bugs:
    * Fixed an issue where the Discounts and Coupons RSS Feed had incorrect title
    * Fixed an issue where a wrong special price expiration date was displayed in RSS
    * Fixed an issue in the Import functionality where imported files disappeared after the Check Data operation
    * Fixed an issue where the Unsubscribe link in the Newsletter was broken
    * Fixed an issue where stock status changed incorrectly after import
    * Fixed an issue where selected filters and exclude did not work during Export
    * Fixed an issue where tax details order was different on order/invoice/refund create and view pages (
    * Fixed a typo in the getCalculationAlgorithm public function
    * Fixed an issue where the incorrect value of Subtotal Including Tax was displayed in invoices
    * Fixed an issue where tax details were not displayed on a new order
    * Improved pricing performance using caching
    * Fixed an issue where CsvImportHandler tests still referring to links from Tax module instead of TaxImportExport module
    * Fixed an issue where an exception was thrown instead of 404 if altering the url for a product with required configuration on the storefront
    * Fixed an issue where the title of successfully placed order page (was empty
    * Fixed an issue where certain fields were not disabled by default on the website scope in System configuration as expected
    * Fixed an issue where third party interfaces were not supported by single-tenant compiler
    * Eliminated the 'protocol' parameter from the ReadInterface and WriteInterface
* GitHub requests:
    * [#979](magento/magento2#979) -- Adding OSL license file name
    * [#978](magento/magento2#978) -- Added ignore rule for media assets in wysiwyg directory
    * [#877](magento/magento2#877) -- Made Topmenu HTML Editable
    * [#906](magento/magento2#906) -- Add tests for View\Layout\Reader\Block and slight refactoring
    * [#682](magento/magento2#682) -- \Magento\Framework\Pricing\PriceCurrencyInterface depends on Magento application code
    * [#581](magento/magento2#581) -- About ByPercent.php under different currencies
    * [#964](magento/magento2#964) -- Improving documentation for jMeter performance tests
    * [#871](magento/magento2#871) -- Replace Symfony2/Yaml in composer
    * [#990](magento/magento2#990) -- add @see annotation before class to make it recognizable by IDE
    * [#988](magento/magento2#988) -- Prevent Varnish from creating cache variations of static files
* Framework improvements:
    * Improved unit and integration tests coverage
mmansoor-magento pushed a commit that referenced this issue Dec 19, 2016
Merge pull request #682 from magento-fearless-kiwis/FearlessKiwis-MAG…
813fad8 
…ETWO-62262

Fearless kiwis magetwo 62262
YevSent pushed a commit to YevSent/magento2 that referenced this issue Jun 12, 2019
magento#682: [Test coverage] added test cases for missed parameters a…
6d71e5c 
…t 'setBillingAddressMutation'
YevSent pushed a commit to YevSent/magento2 that referenced this issue Jun 12, 2019
@vpodorozh
magento#682: [Test coverage] added test cases for missed parameters a…
b8801a0 
…t 'setBillingAddressMutation'

Fix static tests
YevSent pushed a commit to YevSent/magento2 that referenced this issue Jun 12, 2019
@magento-engcom-team
ENGCOM-5209: magento#682: [Test coverage] Missed parameters at 'setBi…
6ea7b69 
…llingAddressMutation' magento#700

 - Merge Pull Request magento/graphql-ce#700 from sergiy-v/graphql-ce:682-missed-parameters-at-set-billing-address-mutation
 - Merged commits:
   1. 6d71e5c
   2. b8801a0
   3. fd7aba9
   4. 91500be
YevSent pushed a commit to YevSent/magento2 that referenced this issue Jun 12, 2019
@lenaorobei
ENGCOM-5209: magento#682: [Test coverage] Missed parameters at 'setBi…
f9eb63f 
…llingAddressMutation' magento#700
YevSent pushed a commit to YevSent/magento2 that referenced this issue Jun 12, 2019
@lenaorobei
ENGCOM-5209: magento#682: [Test coverage] Missed parameters at 'setBi…
4221816 
…llingAddressMutation' magento#700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vpelipenko vpelipenko

Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@maksek @verklov @mcspronko @vpelipenko @joshdifabio @alankent