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

Bugs detected while creating WPML plugin definition files #1317

Closed
wants to merge 8 commits into from
Closed

Bugs detected while creating WPML plugin definition files #1317

wants to merge 8 commits into from

Conversation

mkreckovic
Copy link
Contributor

I can explain every commit if needed.

@borekb
Copy link
Member

borekb commented Feb 13, 2018

Hi @mkreckovic, thanks for the PR, it looks good to me overall. Two commits that will require more attention (cc @JanVoracek) are:

  • TableSchemaStorage#getSchema no longer throwing an exception 46e2669
  • Working with paths of all plugins, not just the active ones 6ce8409

@pavelevap
Copy link
Collaborator

Commit 344e1c1 probably fixes issue #1304.

@pavelevap
Copy link
Collaborator

Hi @mkreckovic,

commit 44b5474 is related to following queries?

INSERT INTO wp_term_relationships ( object_id, term_taxonomy_id ) VALUES ( 1, 2 ),( 2, 2 )

@borekb
Copy link
Member

borekb commented Feb 14, 2018

Copying relevant bits from conversation on Gitter:


Marko Krečković @mkreckovic Feb 13 11:39
Hi @borekb ... regarding TableSchemaStorage#getSchema no longer throwing an exception, we have situation with divi builder plugin where tables for AB testing from divi plugin are not created when activated, but later when plugin needs them. So when we clone that repository and do apply-changes we get error because that tables are in schema file but they don't exists. I think that this change does not impair the functioning of the VP, and allows the plugins that create the tables later during usage, to work properly and don't throw exception.

Marko Krečković @mkreckovic Feb 13 12:09
Also we have situation when VP needs to know table schema before plugin is activated, because plugin activation populates some tables (WPML plugin). Because of that we changed getPathsForActivePlugins with getPathsForPlugins

@mkreckovic
Copy link
Contributor Author

Yes @pavelevap , that's the query. insert_id holds the ID of last inseted row, so firstId will hold wrong value if we have query like that.

@pavelevap
Copy link
Collaborator

Great, so commit 44b5474 probably fixes related issue #1303, I will try to test your suggested fix.

@borekb
Copy link
Member

borekb commented Feb 19, 2018

@mkreckovic: as @pavelevap is reviewing the changes, it would be better to have them in separate PRs. Do you think you could re-submit as such? Sorry to bother you..

@borekb
Copy link
Member

borekb commented Apr 2, 2018

@mkreckovic Thanks for splitting this PR into separate ones. For reference, they are:

@pavelevap, could you please do the first round of reviews on those PRs and ask @JanVoracek for assistance where needed? Thanks!

@mkreckovic Do you think we can close this PR? If everything has been transferred to the new PRs, feel free to do so.

@mkreckovic mkreckovic closed this Apr 9, 2018
@pavelevap
Copy link
Collaborator

I am going through separate PRs and testing them, but one commit from this PR is missing: 44b5474

I tested it before and it did not fix my issue with multiple entities for INSERT query.

@mkreckovic: This commit is no longer relevant or only misses its own PR?

@mkreckovic
Copy link
Contributor Author

@pavelevap I reverted this commit because the problem is more complicated than I originally thought. When we have INSERT ... VALUES, WordPress really returns ID of the first inserted record for the firsId value, but if we have sometnig like VALUES (1, 'First'), (2, 'Second'), (3, 'Third') where 1, 2, and 3 are primary key field, then firstId returns last inserted record.

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.

None yet

3 participants