Skip to content

Conversation

@quisse
Copy link

@quisse quisse commented Apr 4, 2018

Description

Added a method which does nothing but can be used to create a plugin for

Fixed Issues (if relevant)

  1. Adding a handler to the Magento\Catalog\Helper\Output Class #12371: Adding a handler to the Magento\Catalog\Helper\Output Class

Manual testing scenarios

  1. before: https://gist.github.com/quisse/b78ee3d121a4c9b8dd141334c8fa27a8
  2. after: https://gist.github.com/quisse/10e2a5a90f8dc55f82f972efe138e3ad

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@quisse quisse self-assigned this Apr 4, 2018
@quisse quisse requested a review from okorshenko April 4, 2018 08:44
@sidolov sidolov assigned sidolov and unassigned quisse Apr 4, 2018
@sidolov
Copy link
Contributor

sidolov commented Apr 4, 2018

Hi @quisse , I agree with you that current solution hasn't valid point for extending. Adding new empty method that called from the constructor is not a good idea, the mentioned helper is legacy code and doesn't match our Technical guidelines and we don't want to make this code more complicated and unobvious. Current implementation needs more complex solution and refactoring and for now your current implementation the best way to extend \Magento\Catalog\Helper\Output

@sidolov sidolov removed the request for review from okorshenko April 4, 2018 09:22
@magento-engcom-team magento-engcom-team added this to the April 2018 milestone Apr 4, 2018
@quisse
Copy link
Author

quisse commented Apr 4, 2018

Hi @sidolov , i was expecting this isn't the best solution but hey, it's a start 😀. Can you please point me in a direction about how to refactor or shall i close this pull request & remove the non issue label from magento/magento2#12371 so this can be picked up by somebody who knows how to handle?

@quisse
Copy link
Author

quisse commented Apr 4, 2018

Closed this because I've moved this to other branch. I still want to create a new pr with proper refactoring @sidolov .

@quisse
Copy link
Author

quisse commented Jul 4, 2018

@sidolov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants