Mysqldesc for reverse task #821

Merged
merged 5 commits into from Mar 16, 2014

3 participants

@stood

Fix #672 and the test

@stood

I'm sorry my previous commit and my new commit have been blended when compare pull request.
How to fix? Create a new PR?

@staabm
Propel member

you can just squash the commits, within this PR

@staabm staabm and 2 others commented on an outdated diff Jan 29, 2014
generator/lib/reverse/mysql/MysqlSchemaParser.php
@@ -264,6 +265,10 @@ public function getColumnFromRow($row, Table $table)
$column->addVendorInfo($vi);
}
+ if ($desc){
+ $column->setDescription(utf8_encode($desc));
@staabm
Propel member
staabm added a note Jan 29, 2014

did you figure out why this requires utf8 encoding here?

@stood
stood added a note Jan 29, 2014

If you delete utf8 encoding, PHP return :
[PHP Error] DOMElement::setAttribute(): string is not in UTF-8

@staabm
Propel member
staabm added a note Jan 29, 2014

I would expect that $desc is already in utf8... if not the charset of the used sql connection is not correct... or do I miss something? Otherwise we would need this conversation in a lot of places...?

@stood
stood added a note Jan 29, 2014

The query SHOW FULL COLUMNS FROM table has been executed with PDO. And there are just build.properties file for config with just the instructions

propel.project = legacyapp
propel.database = mysql
propel.database.url = mysql:dbname=legacyapp
propel.database.user = root
@staabm
Propel member
staabm added a note Jan 30, 2014

please try with

propel.database.encoding = utf-8

DOMElement requires the data to be UTF-8, so we need to convert whatever the connection charset is into UTF-8...

@jaugustin @willdurand @marcj any suggestions? Do we do such convertion in other places already?

@stood
stood added a note Jan 30, 2014

I already tested this solution and doesn't work.

An other solution : add $this->dbh->query('SET NAMES utf8'); before the query

@marcj
Propel member
marcj added a note Jan 30, 2014

We don't. The issue here is that our Manager classes create connections without the encoding settings.
For example ReverseManager.php:222 -- here is something like that missing:


$con = ConnectionFactory::create(
    array(
        'dsn' => $buildConnection['dsn'], 'user' => $username, 'password' => $password,
        'settings' => array('charset' => $this->getGeneratorConfig()->getBuildProperty('databaseEncoding'))
    ),
    AdapterFactory::create($buildConnection['adapter'])
);

Other Manager classes create the connection on its own as well (=duplicate code). We should consolidate that stuff into AbstractManager.

But another question is: How do we handle wrong encoding? When tables are defined through MySQL Workbench and servers's default encoding is not utf8 for example it's likely that descriptions texts are in latin. When the buildProperties are set then to utf8 then it won't work.
What if the developer created tables at the very beginning with latin1 and then recognized that he was wrong then changed to utf8 -- those description encoding would be mixed.
Am not sure if we should handle and automatically fix those cases or should just presume that the developer configured the properties well and database's content is appropriate.
If not we should throw a meaningful exception when we detect encodings that is different than the current.

So, a utf8_encode call is necessary when database is not configured as utf8 (which may be legit), but that call depends on the current encoding of the database (which can only be retrieved during runtime/extra query).
Since we'd only need those checks in our Manager classes we should place there a method like getConnectionEncoding that calls a equal method on Adapter classes to be cross database compatible. Then we're able to check if we have utf8 data and are able to handle the simplest case of having different encoding than utf8. Mixed encodings are still not handled then.

@staabm
Propel member
staabm added a note Jan 30, 2014

I don't think we should do such time-cosuming things in propel1.

Maybe just mb_detect_encoding

  • if utf8 use as-is
  • if iso8859 convert with utf8_encode
  • otherwise throw exception about unexpected/unsupported encoding

In Propel2 we could do it more thoroughly

@marcj
Propel member
marcj added a note Jan 30, 2014

Yes, only in Propel2.

@staabm
Propel member
staabm added a note Jan 30, 2014

The issue here is that our Manager classes create connections without the encoding settings.

this should be fixed... won't we already mix up data encodings until now if this is the case?

@marcj
Propel member
marcj added a note Jan 30, 2014

I talked actually about Propel2. Haven't seen that we're talking here about Propel1. xD

@staabm
Propel member
staabm added a note Jan 30, 2014

ok, so we agree that in propel1 we could simple do the thing described in #821 (comment) ?

@willdurand agree?

@marcj
Propel member
marcj added a note Jan 30, 2014

Yes, but without the mbstring module dependency. We should not introduce new dependencies for a bugfix-only version. It's actual already too much to include that feature as we have feature freeze in propel 1.x.

@stood
stood added a note Feb 13, 2014

For conclusion, what implement ?

@staabm
Propel member
staabm added a note Feb 13, 2014

@marcj how to detect a utf8 string without mbstring?

@staabm
Propel member
staabm added a note Feb 25, 2014

just did a quick and dirty test and it worked for me... not sure if it will do the job for all sorts of utf8 strings though... but I think it would be a good start...

@stood could you add this check and convert to utf8 only if necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm commented on an outdated diff Jan 29, 2014
...ite/generator/reverse/mysql/MysqlSchemaParserTest.php
@@ -89,6 +89,28 @@ public function testDecimal()
$this->assertEquals($c1->getSize(), $c2->getSize());
$this->assertEquals($c1->getScale(), $c2->getScale());
}
+
+ public function testDescColumn()
+ {
+ $schema = '<database name="reverse_bookstore"><table name="book"><column name="title" type="VARCHAR" size="255" description="Book Title" /></table></database>';
@staabm
Propel member
staabm added a note Jan 29, 2014

please use e.g. a umlaut in the description so we can see why/if the utf8_encoding is required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm and 1 other commented on an outdated diff Jan 29, 2014
.../runtime/formatter/PropelSimpleArrayFormatterTest.php
@@ -29,7 +29,7 @@ public function testFormatWithOneRowAndValueIsNotZero()
$books = $formatter->format($stmt);
$this->assertInstanceOf('PropelCollection', $books);
$this->assertCount(4, $books);
- $this->assertSame('1', $books[0]);
+ $this->assertEquals('1', $books[0]);
@staabm
Propel member
staabm added a note Jan 29, 2014

why was this changed?

@stood
stood added a note Jan 29, 2014

Because PGSQL return an integer and mysql return a string. AssertSame test a type of var

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm and 2 others commented on an outdated diff Jan 29, 2014
.../runtime/formatter/PropelSimpleArrayFormatterTest.php
}
public function testFormatOneWithOneRowAndValueIsNotZero()
{
$con = Propel::getConnection(BookPeer::DATABASE_NAME);
- $stmt = $con->query('SELECT 1 FROM book LIMIT 0, 1');
+ $stmt = $con->query('SELECT 1 FROM book LIMIT 1 OFFSET 0');
@staabm
Propel member
staabm added a note Jan 29, 2014

using which DB backend did you test it... I think this is no longer mysql compatible

@stood
stood added a note Jan 29, 2014

'LIMIT 0, 1' not compatible with PGSQL but ' LIMIT 1 OFFSET 0 ' compatible with mysql . I have tested with mysql and pgsql

@staabm
Propel member
staabm added a note Jan 29, 2014

great!

@marcj
Propel member
marcj added a note Jan 29, 2014

yeah, it's not import because in Propel1 the test suite runs only against mysql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stood

@staabm I have 2 branches and these have been blended in this pr

@staabm
Propel member

@stood are you able to separate those branches again? if not could you just close this and open a new pr with the requested changes in #821 (comment) and #821 (comment) ?

@staabm staabm referenced this pull request Feb 25, 2014
Closed

MysqlDesc for Reverse Task #672

@stood

@staabm It's ok for you ?

@staabm
Propel member

looks good to me, thanks.

@willdurand willdurand merged commit 3f5e010 into propelorm:master Mar 16, 2014

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment