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

Refactor PMA_setGlobalVariablesForEngine #12703

Closed
nijel opened this issue Nov 15, 2016 · 4 comments
Closed

Refactor PMA_setGlobalVariablesForEngine #12703

nijel opened this issue Nov 15, 2016 · 4 comments
Assignees
Milestone

Comments

@nijel
Copy link
Contributor

nijel commented Nov 15, 2016

Currently PMA_setGlobalVariablesForEngine returns long list of variables which are then passed around the code. This is really bad design patter, it should be made more transparent to the users. One of possibilities is to move the code to the Table class and use it from there (with caching).

See also #12567

@arimourao
Copy link
Contributor

How do I assign this issue to me? I will give it a try!

@nijel
Copy link
Contributor Author

nijel commented Nov 17, 2016

You can give it a try :-). AFAIK nobody is working on this.

@arimourao
Copy link
Contributor

I was looking through the code and I think that this can be simplified a lot. There is no need to have several boolean variables in the Table class just to tell if it is a kind of storage engine or not. For instance, I will add a variable in the Table class that will hold the string value of the select storage engine (ie "InnoDb") and instead of

if($is_innoDb){ //Do this }

I can simply have:
if($tableInstance->tbl_storage_engine == 'InnoDb'){ //Do this };
right?

@nijel
Copy link
Contributor Author

nijel commented Nov 18, 2016

I think motivation for this was to avoid doing string comparison too many times as this is something what is used quite often. But maybe in the end this is just overoptimized and string comparison would work fine.

In either case this is used quite often, so I'd prefer some nicer interface to this, for example having Table::isEngine method, which would take string or array (to match against multiple engines) and return boolean.

@nijel nijel closed this as completed in 791a338 Dec 1, 2016
nijel added a commit that referenced this issue Dec 1, 2016
Issue #12703, #12719

Signed-off-by: Michal Čihař <michal@cihar.com>
nijel added a commit that referenced this issue Dec 1, 2016
It did not have the current engine information...

Issue #12703, #12719

Signed-off-by: Michal Čihař <michal@cihar.com>
nijel added a commit that referenced this issue Dec 1, 2016
Issue #12703, #12719

Signed-off-by: Michal Čihař <michal@cihar.com>
nijel added a commit that referenced this issue Dec 1, 2016
Issue #12703, #12719

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel nijel self-assigned this Dec 1, 2016
@nijel nijel added this to the 4.7.0 milestone Dec 1, 2016
nijel added a commit that referenced this issue Dec 1, 2016
Issue #12703, #12719

Signed-off-by: Michal Čihař <michal@cihar.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants