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

[TASK] Splitted Magento1 and Magento2 commands and added Crack_AdminP… #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lewisvoncken
Copy link

…assword Magento 2 Command

@lewisvoncken
Copy link
Author

@vdloo can you please give feedback on this Pull Request?

@vdloo
Copy link
Member

vdloo commented Oct 26, 2018

hi @lewisvoncken I'll take a good look next week, but the first thing I notice is that this basically duplicates a lot of code like from here. Isn't there a better way to do that? Could you also explain a bit in the description of this PR why this change is necessary and what problem it solves? Also please keep your commit titles under the 50 chars.

$this->userModel = $userModel;
$this->storeManager = $storeManager;
$this->encryptor = $encryptor;
$this->filesystem = $filesystem;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdloo This M2 Specific

protected function generateSpecialWordlist()
{
$admins = $this->userModel->getCollection();
$stores = $this->storeManager->getStores();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdloo This M2 Specific

*/
protected function getAdmins()
{
$admins = $this->userModel->getCollection();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdloo This M2 Specific

if ($engineType = $input->getOption('engine')) {
$factory->setEngineType($engineType);
}
$factory->setEncryptor($this->encryptor);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdloo This M2 Specific

{
$path = realpath($this->specialPath);
if (file_exists($path)
&& strpos($path, $this->filesystem->getDirectoryRead('tmp')->getAbsolutePath()) === 0) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdloo This M2 Specific

$generator->setStores($stores);
$words = $generator->generate();

$directoryRead = $this->filesystem->getDirectoryRead('tmp');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdloo This M2 Specific

@lewisvoncken
Copy link
Author

@vdloo I have added comments according the Magento 2 Specific code.

@vdloo
Copy link
Member

vdloo commented Oct 26, 2018

This M2 Specific

right, but I mean isn't there some nice higher level abstraction that can be made so the common parts aren't duplicated?

Copy link

@AlexanderGrooff AlexanderGrooff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code seems to contain a lot of violations of the DRY principle. I would like to see this improved before I can approve this.

@lewisvoncken
Copy link
Author

@vdloo Can you please apply changes as you recommended because the functionality is very useful?

@lewisvoncken
Copy link
Author

@AlexanderGrooff and @vdloo
Can you please merge this PR? This is necessary for security reasons.

@vdloo
Copy link
Member

vdloo commented Feb 15, 2021

@lewisvoncken you asked for feedback on this PR and I mentioned before that in this PR basically a bunch of code is copy-pasted from here and slightly altered. I'm sure there's a nicer way to do that where only the relevant changes for m1 / m2 are abstracted out so that there is not this much duplicate code for everything else that is not different.

I'm OK with merging this if you really need it but I don't think we should copy-paste code like this if there's just minor changes, this makes it hard to spot what exactly is different here from the other unaltered implementation.

$directoryRead = $this->filesystem->getDirectoryRead('tmp');
$dir = $directoryRead->getAbsolutePath('password-cracker');

$io = new \Varien_Io_File;
Copy link
Contributor

@frosit frosit Feb 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this class exists in M2, should be replaced with \Magento\Framework\Filesystem\Io\File; See: https://mage2.pro/t/topic/38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants