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

Fix for #828 #829

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix for #828 #829

wants to merge 2 commits into from

Conversation

deresh
Copy link

@deresh deresh commented Feb 14, 2014

Fix for BC break

#828

Only include global namespace prefix if php version is larger than 5.3.0

@staabm
Copy link
Member

staabm commented Feb 14, 2014

please add a unit test which fails for you under php 5.2.

@stood
Copy link
Contributor

stood commented Feb 15, 2014

why you have commit password and login of database ?

@@ -597,7 +599,7 @@ public static function buildTableMap()
{
\$dbMap = Propel::getDatabaseMap(" . $this->getClassname() . "::DATABASE_NAME);
if (!\$dbMap->hasTable(" . $this->getClassname() . "::TABLE_NAME)) {
\$dbMap->addTableObject(new \\" . $this->getTableMapClass() . "());
\$dbMap->addTableObject(new " . $ns . $this->getTableMapClass() . "());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert everything despite the changes in this file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted.

@staabm
Copy link
Member

staabm commented Feb 18, 2014

@deresh thanks. Could you also add a unit test which fails without your patch?

@deresh
Copy link
Author

deresh commented Feb 18, 2014

@staabm : I'm not sure where to add this test, and now exactly to test for this.
On php5.2 the code returns php Syntax Error, and just fails( fatal error).
Could use some help on this.

@staabm
Copy link
Member

staabm commented Feb 18, 2014

just verified that we are not running the testsuite against php5.2 on travis.

we discussed dropping 5.2 for propel1.7 in #624 but finally discarded the idea.

@deresh could you just add 5.2 to the .travis.yml.
I am curious to see how the testsuite runs against 5.2 ;-)

@deresh
Copy link
Author

deresh commented Feb 18, 2014

added. That should fail on php5.2 withouth this fix

@deresh
Copy link
Author

deresh commented Feb 18, 2014

well now it fails on php 5.2 because build proces requires composer and composer doesn't work on php < 5.3

@staabm
Copy link
Member

staabm commented Feb 18, 2014

not sure how we should handle this case...

@marcj @willdurand any ideas?

Without the testsuite running on 5.2 we could easily regress on php5.2 in the future again.

@willdurand
Copy link
Contributor

We should not test against 5.2, that's all. We have adopted a "best effort" strategy for a while, and we can live with that.

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

4 participants