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

More config-records are selected than it should #787

Merged
merged 5 commits into from May 14, 2020
Merged

More config-records are selected than it should #787

merged 5 commits into from May 14, 2020

Conversation

JaroslavHerber
Copy link
Contributor

The "%" in the LIKE-statement selects more config-records than it should.
For example, if you have 2 themes "mytheme" and "mytheme-mobile" and lets say "mytheme" is the active one, this query selects all configs from both themes, because LIKE "mytheme%" also matches "mytheme-mobile".
Whats the purpose of this "%" anyway?

The "%" in the LIKE-statement selects more config-records than it should.
For example, if you have 2 themes "mytheme" and "mytheme-mobile" and lets say "mytheme" is the active one, this query selects all configs from both themes, because LIKE "mytheme%" also matches "mytheme-mobile".
Whats the purpose of this "%" anyway?
@realFlowControl
Copy link
Contributor

Hey @JaroslavHerber,
thanks for taking the time to make this PR. We will discuss this with the team, why we have this LIKE condition there. In the mean time, could you cleanup this a bit more?

What do you think about this?

/Florian

@JaroslavHerber
Copy link
Contributor Author

Hey @flow-control,

thank you for your comment. Meanwhile I figured out, why the "%" is there. It's because of this const:
const OXMODULE_MODULE_PREFIX = 'module:';

.. which is used in init() to select and load all module-configs at once:
// loading modules config
$this->_loadVarsFromDb($shopID, null, Config::OXMODULE_MODULE_PREFIX);

I'll reconsider a better solution...

Best regards, Jaro

@realFlowControl
Copy link
Contributor

Hey @JaroslavHerber,

we will run this branch through our internal CI and merge if everything is green.
FYI: our internal CI system also runs unit/integration tests with PE and EE editions and several modules installed, as well as Codeception tests.

/Flo

realFlowControl and others added 3 commits May 13, 2020 16:24
This should fix the problem.
The "%" is moved to the only place, where it is needed ->line 405
What do you think?
@realFlowControl
Copy link
Contributor

Hey @JaroslavHerber,

just an update for you: I rebased your pull request on latest master and fixed some minor coding style issues. Also this should fix some tricky issues in travis. Lets hope for the best 🤞

/Flo

@realFlowControl realFlowControl merged commit 87f0376 into OXID-eSales:master May 14, 2020
@realFlowControl
Copy link
Contributor

Hello @JaroslavHerber,

thank you very much, internal CI is also 💚, just merged everything.

/Flo

@JaroslavHerber
Copy link
Contributor Author

Hi @flow-control ,

thats awesome. Thank you very much :)

Best regards,
Jaro

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