Skip to content

Attribute set save admin controller refactor #15990

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

Closed
wants to merge 0 commits into from

Conversation

AnshuMishra17
Copy link
Contributor

Description
Remove direct use of object manager for admin attribute set save controller using constructor based dependency injection.

Manual testing scenarios
Created a new attribute set in admin under Stores -> Attribute Set -> Add Attribute Set.
Edit an existing attribute set under Stores -> Attribute Set

@AnshuMishra17
Copy link
Contributor Author

@ihor-sviziev @sidolov I can see following error message in report.
60 | ERROR | [x] Whitespace found at end of line
61 | ERROR | [ ] Line exceeds maximum limit of 120 characters; contains 128
| | characters
62 | ERROR | [x] Whitespace found at end of line
63 | ERROR | [ ] Line exceeds maximum limit of 120 characters; contains 121
| | characters
64 | ERROR | [x] Whitespace found at end of line
How can I make it according to PSR standard? It would be helpful if you can share any link where I can see any example.
The issue seems to be of the following code
$this->attributeSetFactory = $attributeSetFactory ?:
\Magento\Framework\App\ObjectManager::getInstance()->get(\Magento\Eav\Model\Entity\Attribute\SetFactory::class);
$this->filterManager = $filterManager ?:
\Magento\Framework\App\ObjectManager::getInstance()->get(\Magento\Framework\Filter\FilterManager::class);
$this->jsonHelper = $jsonHelper ?:
\Magento\Framework\App\ObjectManager::getInstance()->get(\Magento\Framework\Json\Helper\Data::class);
}

@ihor-sviziev
Copy link
Contributor

@AnshuMishra17 I added 2 commits that should fix static code failures. Let's wait till travis finish running tests. After I'll process your PR

@ihor-sviziev ihor-sviziev self-assigned this Jun 12, 2018
hitesh-wagento pushed a commit to hitesh-wagento/magento2 that referenced this pull request Jun 12, 2018
hitesh-wagento pushed a commit to hitesh-wagento/magento2 that referenced this pull request Jun 12, 2018
@AnshuMishra17
Copy link
Contributor Author

@ihor-sviziev Thanks. It would be helpful and nice of you if you can explain how did you resolve that and how to squash and force push. I tried following the steps from the link that you have shared earlier but didn't succeed. So, from next time onwards I can raise PR without any issues.

@orlangur
Copy link
Contributor

@AnshuMishra17 @ihor-sviziev please do not hesitate to import at least ObjectManager (Alt+Enter in PhpStorm). It can save some lines :)

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jun 12, 2018

@AnshuMishra17,
prev. time you force pushed empty branch (without any changes). You needed just add your changes as commit and only them force push it.
If you have any questions - feel free to contact me in slack.
Could you also import ObjectManager, as @orlangur described above?

@AnshuMishra17
Copy link
Contributor Author

@ihor-sviziev All checks has been passed. Can please let me know if still any update is required here?
I have imported the ObjectManager as suggested by @orlangur

@ihor-sviziev
Copy link
Contributor

Hi @AnshuMishra17, your changes looks good, but you've changed one file and have 7 commits. Could you squash all your changes in one commit and force push it? Then I'll approve

@ihor-sviziev ihor-sviziev requested review from sidolov and removed request for sidolov June 18, 2018 10:35
@AnshuMishra17
Copy link
Contributor Author

@ihor-sviziev Can you please share the steps or any link where I can read on how to squash the commits. Last time I did that but was unsuccessful.

@AnshuMishra17
Copy link
Contributor Author

@ihor-sviziev Will the following steps work ?
git checkout my_branch
git reset --soft HEAD~7
git commit
git push --force origin my_branch

@ihor-sviziev
Copy link
Contributor

@AnshuMishra17 they should work.

@AnshuMishra17
Copy link
Contributor Author

AnshuMishra17 commented Jun 18, 2018

@ihor-sviziev I have followed following steps/commands but again I was successful.
git branch -d attr-set-save-refactor
git reset --hard upstream/2.2-develop
git push origin 2.2-develop --force
git pull origin 2.2-develop
git checkout -b attr-set-save-refactor
git status
git add -A
git status
git commit -m "Admin controller product set save refactor"
git checkout 2.2-develop
git merge attr-set-save-refactor
git push --force origin 2.2-develop

@ihor-sviziev
Copy link
Contributor

New PR was created with the same changes: #16217

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.

4 participants