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

Added zend-code ^3.0.3 compatibility #63

Merged
merged 6 commits into from
Apr 18, 2018
Merged

Added zend-code ^3.0.3 compatibility #63

merged 6 commits into from
Apr 18, 2018

Conversation

rtek
Copy link
Contributor

@rtek rtek commented Apr 11, 2018

These changes replace various native PHP type aliases with the formal PHP types since zend-code ^3.0 was considering the aliases as actual classes in parameter hints (eg boolean -> \boolean).

References to mixed removed when used in parameter hints but remain in docblocks.

scalar was replaced with int|string in doc blocks since its not a valid PHP type or pseudo-type. is_scalar() defines a scalar as int|string|float|bool but it was used exclusively in an array key context so float|bool are converted to int anyways.

@rtek rtek changed the title Added zend-code ^3.0 compatibility Added zend-code ^3.0.3 compatibility Apr 12, 2018
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

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

Hi! Thanks a lot for the pull request!
Did not expect that upgrading to zend code v3 requires so few changes

composer.json Outdated
@@ -25,14 +25,15 @@
"symfony/config": "^2.2|^3.0|^4.0",
"goetas-webservices/xsd-reader": "^0.2|^0.3.1",
"doctrine/inflector": "^1.0",
"zendframework/zend-code": "~2.3",
"zendframework/zend-code": "~2.3|^3.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

what about getting rid to the v2.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I can do that. Didn't want to alter the deps unless it was necessary.

@@ -34,7 +34,7 @@ public function __construct(array $targetNs, array $aliases = array(), $tmp = nu
$this->phpDir = "$tmp/php";
$this->jmsDir = "$tmp/jms";

$this->namingStrategy = new ShortNamingStrategy();
$this->namingStrategy = new VeryShortNamingStrategy();
Copy link
Member

@goetas goetas Apr 13, 2018

Choose a reason for hiding this comment

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

why changing this? and why a new naming strategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test case using the OTA schema was resulting in so many nested directories that mkdir was failing for being "too long". I put the strategy in the test directory as a workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GoetasWebservices\Xsd\XsdToPhp\PathGenerator\PathGeneratorException: Can't create the 'C:\Users\.........\AppData\Local\Temp/php/ota/RFPResponseType/RFPResponseSegmentsAType/RFPResponseSegmentAType/SitesAType/SiteAType/DatesAType/DateAType/RoomBlockAType/StayDatesAType/StayDateAType/StayDateRoomsAType/StayDateRoomAType/RatesAType' directory: 'mkdir(): Filename too long' in C:\server\root\oss\xsd2php\src\Php\PathGenerator\Psr4PathGenerator.php:19

@@ -34,7 +34,7 @@ public function __construct(array $targetNs, array $aliases = array(), $tmp = nu
$this->phpDir = "$tmp/php";
$this->jmsDir = "$tmp/jms";

$this->namingStrategy = new ShortNamingStrategy();
$this->namingStrategy = defined('PHP_WINDOWS_VERSION_BUILD') ? new VeryShortNamingStrategy() : new ShortNamingStrategy();
Copy link
Member

Choose a reason for hiding this comment

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

👍

@goetas
Copy link
Member

goetas commented Apr 15, 2018

@Ocramius, do you mind having a look at this? Is this enough to use zend code 3?
It looks you were working on something similar


"psr/log": "^1.0"
},
"require-dev": {
"phpunit/phpunit": "^4.8|^5.0",
"jms/serializer": "^1.3",
"goetas-webservices/xsd2php-runtime": "^0.2.7"
"jms/serializer": "^1.9",
Copy link
Member

Choose a reason for hiding this comment

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

just as info, are we using some specific jms 1.9 feature?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, found.

@goetas
Copy link
Member

goetas commented Apr 15, 2018

Looks good to me, will test it a bit better and hopefully merge it this week!

Thanks again for contributing

Copy link

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Besides nits and signatures for the added methods (should really just use PHP 7.1+), looks good!

{
if ($name = $this->classify($type->getName())) {
if (substr($name, -4) !== 'Type') {
$name .= "T";

Choose a reason for hiding this comment

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

Just return early instead of relying on else/else if

* @param Type $type
* @return string
*/
public function getTypeName(Type $type)

Choose a reason for hiding this comment

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

@goetas can't we bump to 7.1 and start having type declarations for these?

@goetas goetas merged commit 4ae6efb into goetas-webservices:master Apr 18, 2018
@goetas
Copy link
Member

goetas commented Apr 18, 2018

thanks 👍

@LeeSaferite
Copy link

@goetas Any chance there will be a new tag with this PR merged soon?

@goetas
Copy link
Member

goetas commented Sep 20, 2018

https://github.com/goetas-webservices/xsd2php/releases/tag/v0.3.3

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.

4 participants