Added test to check wrong constant names built from single inheritance k... #843

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants

acim commented Mar 18, 2014

These requires at the begining are probably not needed, but I needed them to be able to run this single test.

Member

staabm commented Mar 18, 2014

cool. If you already know how to fix the issue you may also add the patch as a second commit.

Member

staabm commented Mar 18, 2014

A good fix should replace any chars which are not allowed in a php-constant by _, but also make sure we won't get something with several _ following each other.

So hallo-_-123 should become hello_123 not hello___123.

Feel free to ask for further assistance.

acim commented Mar 18, 2014

I agree regaring the repetitive chars, they should be consilidated. And yes, I will try to fix this although I didn't dig deep into Propel up to this moment. It is maybe the right time to start :)

acim commented Mar 19, 2014

I made the fix and the unit test passes now, but I have to check it in the real om generation before the request for pull.

@marcj marcj and 2 others commented on an outdated diff Mar 19, 2014

generator/lib/model/Inheritance.php
@@ -34,7 +34,9 @@ class Inheritance extends XMLElement
*/
protected function setupObject()
{
- $this->key = $this->getAttribute("key");
+ // Clean key from special characters not allowed in constant names
+ $key = preg_replace('/[^\w_]/', '_', $this->getAttribute("key"));
+ $this->key = rtrim(preg_replace('/_{2,}/', '_', $key), '_');
@marcj

marcj Mar 19, 2014

Owner

$this->key = preg_replace('/[^\w_]+/', '_', $this->getAttribute("key")); would be easier

@staabm

staabm Mar 19, 2014

Member

but this won't strip duplicate _?

var_dump(preg_replace('/[^\w_]+/', '_', 'hallo-_-123')); // -> 'hallo___123'

@marcj

marcj Mar 19, 2014

Owner

I meant preg_replace('/(\W|_)+/', '_', 'hallo-_-123')

$this->key = preg_replace('/(\W|_)+/', '_', $this->getAttribute("key"));

@staabm

staabm Mar 19, 2014

Member

ahh cool... didn't thought of that.. this seems to work

@acim

acim Mar 19, 2014

Nice, except it is not removing trailing underscore. Shall we remove the trailing underscore at all?

print preg_replace('/(\W|_)+/', '_', 'A0-_-A-');
@staabm

staabm Mar 19, 2014

Member

trailing _ are ulgy, please rtrim them

acim commented Mar 19, 2014

OK, then it should be like this?

- $this->key = $this->getAttribute("key");
+ $this->key = rtrim(preg_replace('/(\W|_)+/', '_', $this->getAttribute("key")), '_');

Or split it in two lines for cleanness?

Member

staabm commented Mar 19, 2014

one line is fine IMO

looks like some _ is missing from the regex

acim commented Mar 19, 2014

No, the _ was there, but Github markdown removed it. I put it in code box now.

OK, I will change this and check the code generation later today and let you know how it passed.

acim commented Mar 19, 2014

Seems to be working, OK, what's next? :)

@marcj marcj and 1 other commented on an outdated diff Mar 19, 2014

...erator/builder/om/GeneratedObjectConstantNameTest.php
+ <inheritance key="Key.-_:*" class="UserCheckMacAddress" extends="UserCheck3"/>
+ </column>
+ </table>
+</database>
+XML;
+ $this->assertEmptyBuilderOutput($schema);
+ }
+
+ protected function assertEmptyBuilderOutput($schema)
+ {
+ $builder = new PropelQuickBuilder();
+ $builder->setSchema($schema);
+
+ ob_start();
+ $builder->buildClasses();
+ $output = preg_replace('/[\r|\n]/', '', ob_get_contents());
@marcj

marcj Mar 19, 2014

Owner

why | here?

@acim

acim Mar 19, 2014

Haven't been using regexes for some time :) I will fix this tomorrow. I wanted to include clearing \r if someone is using Propel on Windows.

@acim

acim Mar 19, 2014

Shall I patch branch 2.0 also?

@marcj marcj commented on the diff Mar 19, 2014

...erator/builder/om/GeneratedObjectConstantNameTest.php
+<database name="constant_name_test" namespace="ConstantNameTest2" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://xsd.propelorm.org/1.6/database.xsd">
+ <table name="radcheck" phpName="UserCheck2">
+ <column name="id" type="INTEGER" sqlType="int(11) unsigned" primaryKey="true" autoIncrement="true" required="true"/>
+ <column name="attribute" type="VARCHAR" size="64" required="true" inheritance="single">
+ <inheritance key="Calling-Station-Id" class="UserCheckMacAddress" extends="UserCheck2"/>
+ </column>
+ </table>
+</database>
+XML;
+ $this->assertEmptyBuilderOutput($schema);
+ }
+
+ /**
+ * Test string with special characters as single inheritance key
+ */
+
@marcj

marcj Mar 19, 2014

Owner

linebreak

Member

staabm commented Mar 19, 2014

@acim good job, thanks.

It also looks good to me, @marcj or @willdurand will merge it, in case there are no further comments

acim commented Mar 20, 2014

There is one more problem:

[Thu Mar 20 08:22:46 2014] [error] [client 10.100.16.3] PHP Fatal error:  Undefined class constant 'CLASSKEY_EXPIRATION' in /var/www/xxx.yyy.net/wisper/models/radius/om/BaseUserCheckExpirationQuery.php on line 46, referer: https://xxx.yyy.net/wisper/index.php

So, CLASSKEY_EXPIRATION constant is not defined for some reason, I will check and fix.

P.S. Actually I think I know where is the problem. There is one comparison which fails now because at one side there is fixed key and on another just uppercased.

acim commented Mar 20, 2014

I haven't deployed newly generated classes to my server, so this is the reason for the problem. I checked generated classes just on my laptop yesterday. Anyway, I have to check this a little bit more and confirm weather everything is fine. I will deploy the changes to the server and see what is going on.

When I said I know what it can be, I actually suspected to condition in line 636 of PHP5PeerBuilder.php, but it should be fine. My classnames and keys are not the same, so this condition should be always fullfilled and the constant should be made.

acim commented Mar 21, 2014

I deployed newly generated models to my server last night and everything seems to be fine, so far so good :)

Owner

willdurand commented Mar 21, 2014

could you squash your commits?

acim commented Mar 21, 2014

Done and sent new pull request.

willdurand closed this Mar 21, 2014

acim referenced this pull request in propelorm/Propel2 Nov 12, 2014

Closed

Single inheritance problem with constant's names #804

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