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

xml: support table 'opt' attribute with mysqli #267

Closed
wants to merge 1 commit into from

Conversation

@peterdd
Copy link
Contributor

@peterdd peterdd commented Aug 10, 2016

This is a quickfix just to make it work without impact on other things.
Currently the opt tag is just ignored for mysqli!

@dregad
Copy link
Member

@dregad dregad commented Sep 8, 2016

This feels like a hack... I'm wondering (keep in mind that I don't really use XML schema) if it wouldn't be more appropriate to use $this->parent->db->dataProvider instead ?

@mnewnham
Copy link
Contributor

@mnewnham mnewnham commented Sep 10, 2016

I agree, My own experience of having to migrate from mysqlt to mysqli in xmlschema files makes me believe that the dataprovider should always be used for xmlschema internals, so it should be:

 $this->opts[$this->parent->db->dataProvider] = $opt;

@dregad dregad changed the title support opt of table for xmlschema03 with mysqli xml: support table 'opt' attribute with mysqli Sep 14, 2016
@dregad dregad added this to the v5.21 milestone Sep 14, 2016
@dregad dregad self-assigned this Sep 14, 2016
@dregad dregad closed this in 27bcd7d Sep 14, 2016
@dregad
Copy link
Member

@dregad dregad commented Sep 15, 2016

Reopening this, following 27bcd7d#commitcomment-19029593.

@peterdd

switching to 'dataProvider' would probably break other db drivers usage in xmlschema03 platform attributes

Are you recommending that I revert 27bcd7d ? As mentioned previously I don't use XML schema much, so I just went with @mnewnham's reply, since you didn't object to it.

@dregad dregad reopened this Sep 15, 2016
@mnewnham
Copy link
Contributor

@mnewnham mnewnham commented Sep 17, 2016

Point taken about backward compatibility. Perhaps something where we could match either the databaseType or dataProvider might work.

I personally believe that moving forward, we should have a dataprovider option and the users should be encouraged to use that (to avoid the issues that I myself encountered).

Just looking at the code in addTableOpt

if(isset($this->currentPlatform)) {
    $this->opts[$this->parent->db->databaseType] = $opt;
}

This looks problematic because $this->currentPlatform is defined as a class variable, therefore it always exists. It is however set true/false by the constraint tag (but only this). This feels almost like incomplete code that needs deeper investigation.

@dregad
Copy link
Member

@dregad dregad commented Jan 5, 2020

@peterdd you have never replied to #267 (comment).

As it stands, commit 27bcd7d is part of master branch, should it be reverted or not ? It's worth noting that noone reported any issues in over 3 years, following the switch to use of dataProvider instead of databaseType.

What do I do with the PR ? Merge this, cancel or postpone to later release ?

@peterdd
Copy link
Contributor Author

@peterdd peterdd commented Jan 5, 2020

Do I get time until next weekend?

@dregad
Copy link
Member

@dregad dregad commented Jan 5, 2020

I was hoping to finish this week-end, but considering that 5.21 has been pending for about 3 years, a week is not going to make much difference 😉

@dregad
Copy link
Member

@dregad dregad commented Jan 12, 2020

Do I get time until next weekend?

@peterdd ping

@peterdd
Copy link
Contributor Author

@peterdd peterdd commented Jan 15, 2020

Sorry, got sidetracked by testing more basic xmlschema03 and session2 stuff (got sucked into the rabbithole) and RL.

I try, but I better do set no due date when focus on this. :-/

@dregad
Copy link
Member

@dregad dregad commented Jan 16, 2020

got sucked into the rabbithole) and RL

I know what it's like, trust me ;-)

I am fine if you don't have time to fully analyze and provide a proper solution, we can delay that to some later date. For now, I just need to know what I should do with 5.21.0

  1. revert 27bcd7d, i.e. the "safe" option (i.e. no support for opt attribute on mysqli in 5.21.0
  2. keep the change in and release as-is, i.e. we accept the risk that other drivers could be broken (as you explained in 27bcd7d#commitcomment-19029593)

The risk is quite low as it only affects XML feature, which is not so widely used.

Since you're the original requestor of the feature, and also the one who's worried about the risk, I guess it's your call, just tell me what you want me to do.

@dregad
Copy link
Member

@dregad dregad commented Jan 24, 2020

@peterdd without feedback from you, I'll go with option 2, i.e. keep the change in and release as-is. Please respond by Sunday evening.

@dregad dregad removed this from the v5.21.0 milestone Feb 1, 2021
@dregad dregad added this to the v5.22.0 milestone Feb 1, 2021
@dregad
Copy link
Member

@dregad dregad commented Apr 22, 2021

@peterdd since 5.21.0 has been released and you never followed up with this discussion, I'm going to close the PR. Feel free to reopen it if needed.

@dregad dregad closed this Apr 22, 2021
@peterdd
Copy link
Contributor Author

@peterdd peterdd commented Apr 22, 2021

Not forgotten, shift to 5.22+

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

Successfully merging this pull request may close these issues.

None yet

3 participants