Skip to content

Add db-prefix from env conf when command admin:user:create is executed #11199

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

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

osrecio
Copy link
Member

@osrecio osrecio commented Oct 3, 2017

Read config env.php to get db-prefix when command admin:user:create is executed

Description

If you don't read db-prefix in env.php you don't have this information in this function: Magento\Setup\Model\AdminAccount::getTableName that is called in first instance in\Magento\Setup\Model\Installer::installAdminUser.

Fixed Issues (if relevant)

  1. Configured table prefix is not recognized in CLI admin:user:create #11176: IConfigured table prefix is not recognized in CLI admin:user:create

Manual testing scenarios

  1. Install fresh Magento with db-prefix
  2. Create admin user: php bin/magento admin:user:create --admin-user=user --admin-password=pass123Lolailo [email protected] --admin-firstname=fname --admin-lastname=lname

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)

@osrecio osrecio changed the title Add db-prefix from env conf Add db-prefix from env conf when command admin:user:create is executed Oct 3, 2017
@orlangur
Copy link
Contributor

orlangur commented Oct 3, 2017

Is it an easiest way to make $this->setup->getTable('admin_user') return correct table name? I doubt it's a good idea to do it in some particular command.

@osrecio
Copy link
Member Author

osrecio commented Oct 3, 2017

Where would you put $this->setup->getTable('admin_user') in this case ?
I mean, you don't have access to assign db-prefix in other part that inside the command.

PS: maybe i didn't understand your question

@orlangur
Copy link
Contributor

orlangur commented Oct 3, 2017

I'm referring to the underlying \Magento\Setup\Model\AdminAccount::saveAdminUser. Can we inject db-prefix on a higher level somehow so that there is no need to do it in each affected command?

@osrecio
Copy link
Member Author

osrecio commented Oct 3, 2017

Ahh Ok, Maybe would be better approach, but in this case, Only found a reference for \Magento\Setup\Model\Installer::installAdminUser in this command.

@orlangur
Copy link
Contributor

orlangur commented Oct 3, 2017

What about any other interactions with database?

@osrecio
Copy link
Member Author

osrecio commented Oct 3, 2017

Yeah, I know, but in Internal Ticket: MAGETWO-62966 was changed Magento\Setup\Module\Setup by Magento\Framework\DB\Adapter\AdapterInterface in this file AdminAccountFactory.php.

I can change again but I don't know certainly is good idea.

$installer->installAdminUser($input->getOptions());
$options = $input->getOptions();
$options['db-prefix'] = $this->deploymentConfig->get(ConfigOptionsListConstants::CONFIG_PATH_DB_PREFIX);
$installer->installAdminUser($options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix must be moved into model \Magento\Setup\Model\Installer::installAdminUser. This class is already aware of deploymentConfig, all occurrences of DB_PREFIX are already in models.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added Magento\Setup\Module\Setup to Interface AdminAccountFactory inside installAdminUser.

I hope these changes are ok for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should change AdapterInterface back to Setup.

I suggested just to move DB prefix obtaining from command to \Magento\Setup\Model\Installer::installAdminUser model (not to any other model).

@osrecio
Copy link
Member Author

osrecio commented Oct 4, 2017

Added to Installer the information about db-prefix

@osrecio
Copy link
Member Author

osrecio commented Oct 5, 2017

Squashed commits in one, Thanks @orlangur

@orlangur
Copy link
Contributor

orlangur commented Oct 5, 2017

Great! 👍

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.

5 participants