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

Upgrade ZF components. Zend_Service #9243

Closed
okorshenko opened this issue Apr 13, 2017 · 5 comments
Closed

Upgrade ZF components. Zend_Service #9243

okorshenko opened this issue Apr 13, 2017 · 5 comments
Assignees
Labels
Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed up for grabs

Comments

@okorshenko
Copy link
Contributor

okorshenko commented Apr 13, 2017

Description
Upgrade components from ZF1 to ZF2

As a long term goal, we would like to eliminate knowledge about 3rd party libraries from Magento code base. Magento code still can use 3rd party libraries, but they must be wrapped by Magento interfaces and classes (adapters) so that 3rd party libraries can be easily substituted by newest versions or alternative implementations.

Acceptance Criteria

  1. Magento interfaces are defined and can be used instead of ZF1 classes in Magento code
  2. Default implementations for new interfaces are defined and configured as the preferences for new interfaces
  3. Client code of the ZF1 components is refactored to use Magento interfaces
  4. Zend classes are used as private services in Magento adapters
  5. Backward compatibility is preserved
  6. ZF1 components are updated to ZF2

List of the components

# Component Name
(total usages count)
Module Name Usages Count
1 Zend_Service (total: 1) Framework\CurrencyInterface.php 1
@dverkade
Copy link
Member

dverkade commented Jun 12, 2017

Hi @okorshenko

I did some digging into this issue and here is what I found.

As far as I can see the mention to \Zend_Service is incorrect and might be a bug/wrong in Zend Framwork. If you look into the \Zend_Currency class you see that it only allows for Zend_Currency_CurrencyInterface classes to be passed to the setService function. So the getService can only return a class which implements this interface.

public function setService($service)
    {
        if (is_string($service)) {
            #require_once 'Zend/Loader.php';
            if (!class_exists($service)) {
                $file = str_replace('_', DIRECTORY_SEPARATOR, $service) . '.php';
                if (Zend_Loader::isReadable($file)) {
                    Zend_Loader::loadClass($service);
                }
            }

            $service = new $service;
        }

        if (!($service instanceof Zend_Currency_CurrencyInterface)) {
            #require_once 'Zend/Currency/Exception.php';
            throw new Zend_Currency_Exception('A currency service must implement Zend_Currency_CurrencyInterface');
        }

        $this->_options['service'] = $service;
        return $this;
    }

I was also checking how Magento handles the fetching of the currency rates (that is what these functions are for) from and external service. I see this is done in the \Magento\Directory\Model\Currency\Import namespace and does not go truth this code.

How do you want to handle this?

  • We can change the comment in the Framework\CurrencyInterface.php so that the comment will reflect the actual interface being returned, which is: Zend_Currency_CurrencyInterface
  • Maybe we can deprecate the getService and setService functions because they are not used.
  • Other solution?

@magento-engcom-team magento-engcom-team added Application Framework Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed up for grabs labels Sep 11, 2017
@magento-engcom-team
Copy link
Contributor

No activity. Closing the issue

@okorshenko
Copy link
Contributor Author

Hi @dverkade
Since this is @api interface we cannot remove any methods from the contract.
We need to modify doc blocks for the following methods:

    /**
     * Returns the set service class
     *
     * @return \Zend_Service
     */
    public function getService();

    /**
     * Sets a new exchange service
     *
     * @param string|\Magento\Framework\Locale\CurrencyInterface $service Service class
     * @return \Magento\Framework\CurrencyInterface
     */
    public function setService($service);
  1. Remove Magento\Framework\Locale\CurrencyInterface from setService method and add align it with \Zend_Currency signatures
  2. Remove \Zend_Service from getService method and add align it with \Zend_Currency signatures
  3. Add @deprecated tags to both methods and add @see annotation to the methods. Please, reference corresponding interface from \Magento\Directory\Model\Currency\Import namespace that must be used instead these methods
  4. We would like to cleanup entire interface, we can't deprecate other methods without suggested alternative implementations. So let's stop on these two methods for now

It would be great if you can submit PR to 2.2-develop branch.

Please, let me know if you have any questions.

@magento-team
Copy link
Contributor

Hi @okorshenko. Thank you for your report.
The issue has been fixed in #12957 by @dverkade in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming patch release.

@magento-team magento-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Jan 3, 2018
magento-team pushed a commit that referenced this issue Jan 3, 2018
…nents. Zend_Service #12957

 - Merge Pull Request #12957 from dverkade/magento2:2.3-Upgrade-ZF-components.-Zend_Service
 - Merged commits:
   1. 68389d6
magento-team pushed a commit that referenced this issue Jan 3, 2018
@okorshenko okorshenko added the Fixed in 2.1.x The issue has been fixed in 2.1 release line label Jan 8, 2018
@okorshenko
Copy link
Contributor Author

Hi @okorshenko. Thank you for your report.
The issue has been fixed in #12958 by @dverkade in 2.1-develop branch
Related commit(s):

The fix will be available with the upcoming patch release.

magento-team pushed a commit that referenced this issue Jan 8, 2018
…nents. Zend_Service #12958

 - Merge Pull Request #12958 from dverkade/magento2:2.1-Upgrade-ZF-components.-Zend_Service
 - Merged commits:
   1. abc7bc6
magento-team pushed a commit that referenced this issue Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed up for grabs
Projects
None yet
Development

No branches or pull requests

4 participants