Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

WSDL Generation rewrite (with new tests also) as a base for future changes. #3919

Closed
wants to merge 21 commits into from

4 participants

@GrzegorzDrozd

Hello.

Serious Rewrite of WSDL generation from strings and simple DOM to full object oriented from the begining. This is a base for more changes like: multiple faults, missing xsd types etc.
It uses dom and proper namespaces. All tests are also rewritten to new structure and now are using xpath versus old string comparison.
Code formating in accordance with Coding Style.
More tests.
Integration of recent code changes introduced among others by ayastreb and weierophinney.
Even more tests and test data.

I checked current test output wsdl files against new xpath rules and everything was in green. I also checked new wsdl files against old tests.

I have more patches almost ready and all of them are based on this rewrite.

This is my first pull request so if I did something wrong please let me know so I can fix it.

Best regards.

Grzegorz Drozd and others added some commits
Grzegorz Drozd Changes in a way that WSDL is generated - dom and objects. 6306a57
Grzegorz Drozd More DOM f0be02a
Grzegorz Drozd Merge remote-tracking branch 'upstream/master'
Conflicts:
	library/Zend/Soap/Wsdl.php
	tests/Zend/Soap/Server/DocumentLiteralWrapperTest.php
	tests/Zend/Soap/WsdlTest.php
8476ba1
Grzegorz Drozd Fixes in wsdl generation and tests. f2e03de
Grzegorz Drozd More code changes 73efbc2
Grzegorz Drozd Old files removed ab0e58b
Grzegorz Drozd Complete rewrite of wsdl generation.
Complete rewrite of wsdl generation from strings and simple dom to full object oriented from the begining.
It uses dom and proper namespaces. All tests are also rewritten to new structure and now are using xpath
versus old string comparison.
e37b943
Grzegorz Drozd Merge branch 'master' of github.com:GrzegorzDrozd/zf2
Conflicts:
	library/Zend/Soap/Wsdl.php
	tests/Zend/Soap/WsdlTest.php
4392452
Grzegorz Drozd Quick fix in test.
When runing clean from command line one test was failing. Fixed.
b784880
Grzegorz Drozd Further improvements to the code.
Code formating in accordance with Coding Style.
More tests.
Integration of recent code changes from ayastreb and weierophinney.
Even more tests and test data.
b84e979
Grzegorz Drozd Missing tests for DotNet client. a6edf8b
@Maks3w
Collaborator

Your commits cannot be merged because are in conflict with the actual master branch. For fix this merge the most recent version of master in your PR branch.

Also it's a good thing make your commits in a specific branch rather than master or develop. See this article for to see how to do this http://blog.evan.pro/keeping-a-clean-github-fork-part-1

Finally some files has now different chmod rigts (00644 > 00755) please restore the original chmod rights

library/Zend/Soap/Wsdl.php
@@ -285,50 +353,48 @@ public function addBinding($name, $portType)
* Add an operation to a binding element
*
* @param object $binding A binding XML_Tree_Node returned by {@link function addBinding}
- * @param array $input An array of attributes for the input element, allowed keys are: 'use', 'namespace', 'encodingStyle'. {@link http://www.w3.org/TR/wsdl#_soap:body More Information}
- * @param array $output An array of attributes for the output element, allowed keys are: 'use', 'namespace', 'encodingStyle'. {@link http://www.w3.org/TR/wsdl#_soap:body More Information}
- * @param array $fault An array of attributes for the fault element, allowed keys are: 'name', 'use', 'namespace', 'encodingStyle'. {@link http://www.w3.org/TR/wsdl#_soap:body More Information}
+ * @param string $name
@Maks3w Collaborator
Maks3w added a note

Check this kind of indentations along all the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/AutoDiscover.php
@@ -106,10 +109,11 @@ class AutoDiscover
* @param string $wsdlClass
* @param array $classMap
*/
- public function __construct(ComplexTypeStrategy $strategy = null, $endpointUri=null, $wsdlClass=null, array $classMap = array())
- {
+ public function __construct(ComplexTypeStrategy $strategy = null,
+ $endpointUri=null, $wsdlClass=null, array $classMap = array()
@Maks3w Collaborator
Maks3w added a note

According PSR-2 guidelines when the arguments line is splitted must be only one argument by line

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#44-method-arguments

So PSR-2 is now standard for Zend Framework ? Because in coding standard there is no info about one argument per line: http://framework.zend.com/wiki/display/ZFDEV2/Coding+Standards#CodingStandards-FunctionandMethodDeclaration

@Maks3w Collaborator
Maks3w added a note

Yes, PSR-2 is the statndard for ZF2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff
library/Zend/Soap/AutoDiscover.php
((5 lines not shown))
* @return AutoDiscover
*/
public function addFunction($function)
{
- $this->functions[] = $function;
+ if (is_array($function)) {
+ foreach($function as $row){
+ $this->addFunction($row);
+ }
+ } elseif (is_string($function)) {
+ if (function_exists($function)) {
@Maks3w Collaborator
Maks3w added a note

Is this checking a method inside of an object?

No. AddFunction is used to add standalone function - not method. So function_exists is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/AutoDiscover.php
((7 lines not shown))
*/
public function setUri($uri)
{
if (!is_string($uri) && !($uri instanceof Uri\Uri)) {
throw new Exception\InvalidArgumentException(
- 'No uri given to \Zend\Soap\AutoDiscover::setUri as string or \Zend\Uri\Uri instance.'
+ 'Argument to \Zend\Soap\AutoDiscover::setUri should be string '
@Maks3w Collaborator
Maks3w added a note

Don't be so dramatic with the line length. PSR coding style guidelines doesn't have a hard limit so if you can read the line at once in the GitHub's review pane then is good (around 120 - 140 characters length)

OK. I will reformat code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/AutoDiscover.php
((7 lines not shown))
*/
public function getUri()
{
if ($this->uri === null) {
- throw new Exception\RuntimeException("Missing uri. You have to explicitly configure the Endpoint Uri by calling AutoDiscover::setUri().");
+ throw new Exception\RuntimeException("Missing uri. You have to '
@Maks3w Collaborator
Maks3w added a note

Instead split the string just move the argument to a new line

OK. I will reformat code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/AutoDiscover.php
((5 lines not shown))
* @return AutoDiscover
*/
public function addFunction($function)
{
- $this->functions[] = $function;
+ if (is_array($function)) {
+ foreach($function as $row){
+ $this->addFunction($row);
+ }
+ } elseif (is_string($function)) {
+ if (function_exists($function)) {
+ $this->functions[] = $function;
+ } else {
+ throw new Exception\InvalidArgumentException('Argument to '
+ . '\Zend\Soap\AutoDiscover::addFunction should be a valid function name.'
@Maks3w Collaborator
Maks3w added a note

Same things about split lines. Also you can avoid the leading slash

OK. I will reformat code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Client.php
((8 lines not shown))
*/
public function setClassmap(array $classmap)
{
foreach ($classmap as $class) {
if (!class_exists($class)) {
- throw new Exception\InvalidArgumentException('Invalid class in class map');
+ throw new Exception\InvalidArgumentException('Invalid class in class map: '.$class);
@Maks3w Collaborator
Maks3w added a note

Add spaces around concat operator .

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((9 lines not shown))
case 'featues':
- trigger_error(__METHOD__ . ': the option "featues" is deprecated as of 1.10.x and will be removed with 2.0.0; use "features" instead', E_USER_NOTICE);
+ trigger_error(__METHOD__ . ': the option "featues" is deprecated'.
@Maks3w Collaborator
Maks3w added a note

You can remove case

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

I just committed merge with master and file permission fix.

@Maks3w Maks3w commented on the diff
library/Zend/Soap/Server.php
@@ -329,6 +357,7 @@ public function setActor($actor)
{
$this->validateUrn($actor);
$this->actor = $actor;
+
return $this;
}
@Maks3w Collaborator
Maks3w added a note

Please restore the return type

Do you mean change from @return bool to @return true ? But TRUE is not valid data type. Maybe I should change is to : "@return bool always true or throws exception when error."

@Maks3w Collaborator
Maks3w added a note

true is a valid phpDoc type. A different thing is that some IDE's doesn't support it

Are you sure?
http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.return.pkg.html
States: "The datatype should be a valid PHP type (int, string, bool, etc), a class name for the type of object returned, or simply "mixed". "

Thank you for a link :) I did not know that phpdoc and phpdocumentator differ about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
((40 lines not shown))
$operation->appendChild($node);
+ $node->setAttribute('message', $fault);
}
@Maks3w Collaborator
Maks3w added a note

Is really necessary use and instead &&?

Changed to &&

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/ZendTest/Soap/WsdlTest.php
((6 lines not shown))
-use Zend\Soap\Wsdl;
@Maks3w Collaborator
Maks3w added a note

Why change this?

I think this is from my editor: "Organize imports".
Strange because later in code full form and shorter is used. Mostly in testGetComplexTypeBasedOnStrategiesStringNames and testGetComplexTypeBasedOnStrategiesBackwardsCompabilityBoolean. I will change it to same usage.

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

Code is reformatted according to Your suggestions: multi-line arguments and more loose line length (only when line length excess 120 chars new line is introduced.

Maybe it is time to change guide for contributors? It still points to outdated code style rules?

I don't know how to move commits to other branch. Is this necessary? My next PR are in separate branches, only this one is on master because I started to work on it so long ago.

@Maks3w
Collaborator

it's not imprescindible the use of another branch. Just keep in mind don't do any other things with the branch until it's merged (sometimes the merge decision can take some weeks)

library/Zend/Soap/AutoDiscover.php
@@ -544,13 +618,4 @@ public function toXml()
{
return $this->generate()->toXml();
}
-
- /**
- * Handle WSDL document.
- */
- public function handle()
@Maks3w Collaborator
Maks3w added a note

Why remove this? This functionality was added recently.

By mistake I moved it to other branch. I will restore it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Client.php
((13 lines not shown))
+ throw new Exception\InvalidArgumentException(
+ 'Invalid from_xml callback for type: ' . $type['type_name']
+ );
+ }
+ if (!is_callable($type['to_xml'])) {
+ throw new Exception\InvalidArgumentException(
+ 'Invalid to_xml callback for type: ' . $type['type_name']
+ );
+ }
+ }
+
+ $this->typemap = $typeMap;
+ $this->soapClient = null;
+
+ return $this;
+}
@Maks3w Collaborator
Maks3w added a note

indentantion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((7 lines not shown))
case 'cache_wsdl':
$this->setWSDLCache($value);
break;
+
+ // @todo is missing break really necessary ?
+ case 'featues':
@Maks3w Collaborator
Maks3w added a note

please remove this case totally.

OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((5 lines not shown))
* @throws Exception\InvalidArgumentException on invalid URN
+ *
+ * @return bool
@Maks3w Collaborator
Maks3w added a note

Remember restore this type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((5 lines not shown))
return $this;
}
/**
* Get server persistence
*
- * @return int
+ * @return Server
@Maks3w Collaborator
Maks3w added a note

uh?

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
@@ -72,6 +60,13 @@ class Wsdl
*/
protected $classMap = array();
+ const NS_WSDL = 'http://schemas.xmlsoap.org/wsdl/';
@Maks3w Collaborator
Maks3w added a note

change constant names is a BC break. Also I prefer to have them on top of the class (they are static)

This constants were introduced during my work on my changes. I will change my constants to use earlier names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
((38 lines not shown))
- * @param int $soapVersion SOAP version to be used in binding operation. 1.1 used by default.
- * @return DOMElement The new Operation's XML_Tree_Node for use with {@link function addSoapOperation} and {@link function addDocumentation}
- */
- public function addBindingOperation(
- $binding,
- $name,
- $input = false,
- $output = false,
- $fault = false,
- $soapVersion = SOAP_1_1
- ) {
- $operation = $this->dom->createElement('operation');
- $operation->setAttribute('name', $name);
+ * @param object $binding A binding XML_Tree_Node returned by {@link function addBinding}
+ * @param string $name
+ * @param array|bool $input An array of attributes for the input element,
@Maks3w Collaborator
Maks3w added a note

indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
((33 lines not shown))
if (!$filename) {
echo $this->toXML();
return true;
}
- return (bool) file_put_contents($filename, $this->toXML());
+
+ $i = file_put_contents($filename, $this->toXML());
+
+ return $i > 0;
@Maks3w Collaborator
Maks3w added a note

This should be a strict check (===) against false (http://es2.php.net/manual/en/function.file-put-contents.php)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Grzegorz Drozd added some commits
Grzegorz Drozd Fixed failing tests. 25c337a
Grzegorz Drozd Proper namespace constants.
Change in namespace prefixes to use constants.
Format and code style fixes acording to code review.
Restpre AutoDiscovery::handle with tests.
8197276
@Maks3w Maks3w commented on the diff
library/Zend/Soap/Wsdl.php
((70 lines not shown))
- $targetNamespace = $this->wsdl->getAttribute('targetNamespace');
- }
- return $targetNamespace;
+ $dom = new \DOMDocument();
+ $dom->preserveWhiteSpace = true;
+ $dom->formatOutput = true;
+ $dom->resolveExternals = false;
+ $dom->encoding = 'UTF-8';
+ $dom->substituteEntities = false;
+
+ $definitions = $dom->createElementNS(Wsdl::WSDL_NS_URI, 'definitions');
+ $dom->appendChild($definitions);
+
+ $uri = $this->sanitizeUri($uri);
+ $this->setAttributeWithSanitization($definitions, 'name', $name);
+ $this->setAttributeWithSanitization($definitions, 'targetNamespace', $uri);
@Maks3w Collaborator
Maks3w added a note

:x: With the new flow $uri is not escaped yet. It's the same variable passed to the constructor and not the uri field which is escaped

@weierophinney Owner

@Maks3w Actually, setAttributeWithSanitization() will (eventually) call sanitizeUri(), so the value is safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
@@ -302,77 +347,78 @@ public function addPortOperation($portType, $name, $input = false, $output = fal
* Add a {@link http://www.w3.org/TR/wsdl#_bindings binding} element to WSDL
*
* @param string $name Name of the Binding
- * @param string $portType name of the portType to bind
- * @return DOMElement The new binding's XML_Tree_Node for use with {@link function addBindingOperation} and {@link function addDocumentation}
+ * @param string $portType name of the portType to bind
+ *
@Maks3w Collaborator
Maks3w added a note

indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
@@ -302,77 +347,78 @@ public function addPortOperation($portType, $name, $input = false, $output = fal
* Add a {@link http://www.w3.org/TR/wsdl#_bindings binding} element to WSDL
*
* @param string $name Name of the Binding
- * @param string $portType name of the portType to bind
- * @return DOMElement The new binding's XML_Tree_Node for use with {@link function addBindingOperation} and {@link function addDocumentation}
+ * @param string $portType name of the portType to bind
+ *
+ * @return object The new binding's XML_Tree_Node for use with {@link function addBindingOperation} and {@link function addDocumentation}
@Maks3w Collaborator
Maks3w added a note

object type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff
library/Zend/Soap/Wsdl.php
((32 lines not shown))
case 'void':
return '';
default:
// delegate retrieval of complex type to current strategy
return $this->addComplexType($type);
- }
+ }
@Maks3w Collaborator
Maks3w added a note

old indentation is the correct. Its the close of switch, instead looks the close of default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
((64 lines not shown))
*/
- public function getTargetNamespace()
@Maks3w Collaborator
Maks3w added a note

Remove a public method is a BC Break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Soap/Wsdl/ComplexTypeStrategy/DefaultComplexType.php
((9 lines not shown))
*/
class DefaultComplexType extends AbstractComplexTypeStrategy
{
/**
- * Add a complex type by recursively using all the class properties fetched via Reflection.
+ * Add a complex type by recursivly using all the class properties fetched via Reflection.
@Maks3w Collaborator
Maks3w added a note

typo

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

@GrzegorzDrozd Sorry for do the review in rounds. It's too huge for review all at once

@GrzegorzDrozd

@Maks3w No problem. At least I have time to fix it :) Or do you want me to stop committing until you check once all ?

@Maks3w
Collaborator

I prefer if you could do a review your self first so you can find your own bugs, cs issues, etc.

library/Zend/Soap/AutoDiscover.php
@@ -194,19 +207,27 @@ public function getServiceName()
/**
- * Set the location at which the WSDL file will be available.
+ * Set the location at which the WSDL file will be availabe.
@Maks3w Collaborator
Maks3w added a note

typo

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

In successive PRs let me suggest you to have one for the runtime code and a different one for the CS. By this way the amount of code changed is more reduced

@GrzegorzDrozd

Hm... ok ... give me 3 hours:
to fix typos
\t => 4xspaces
check old api methods with new (missing methods)
dictionary check etc.
And I will commit all at once. Ok ?

@Maks3w
Collaborator

ok, write a comment with you finish.

Grzegorz Drozd Restored missing function from AutoDiscover
Code style and reformat
9d94647
@GrzegorzDrozd

OK. I'm ready for next round ;)
All typos that aspell could find are fixed.
All tabs for indentation are removed (imho all tabs) and replaced with 4xspaces
getTargetNamespace was last missing public function - grep & diff
Comment's, style and line length fixes.

This should be easier now. And now I know that I should do this first even before PR.

@Maks3w Maks3w commented on the diff
library/Zend/Soap/Client.php
((74 lines not shown))
* @var string
*/
protected $lastMethod = '';
/**
- * SOAP request headers.
@Maks3w Collaborator
Maks3w added a note

I prefer this description since is more generic and not so close to the internal implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Client.php
((81 lines not shown))
* Array of SoapHeader objects
- *
* @var array
@Maks3w Collaborator
Maks3w added a note

You may replace array with SoapHeader[]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Client/Common.php
((14 lines not shown))
* @return mixed
*/
public function __doRequest($request, $location, $action, $version, $oneWay = null)
{
- // ltrim is a workaround for https://bugs.php.net/bug.php?id=63780
@Maks3w Collaborator
Maks3w added a note

:x: Uh, I'm very sure that you don't want remove this unless you're a nightmare freak :)

Hmm ... oversight during merging. Strange because I don't remember merging this file and removing this as I am well aware of this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((7 lines not shown))
* @return \Zend\Soap\Server
*/
public function setOptions($options)
{
- if ($options instanceof Traversable) {
+ if ($options instanceof \Traversable) {
@Maks3w Collaborator
Maks3w added a note

Our rule is import all classes and don't use the global namespace operator.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff
library/Zend/Soap/Server.php
@@ -487,11 +557,13 @@ public function addFunction($function, $namespace = '')
throw new Exception\InvalidArgumentException('One or more invalid functions specified in array');
}
}
- $this->functions = array_merge($this->functions, $function);
@Maks3w Collaborator
Maks3w added a note

:warning: Internal note: This looks actual bug, the code above check if $function is an array and iterate over it validating each value and appending to the functions field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((13 lines not shown))
$xml = $request->saveXML();
- } elseif ($request instanceof DOMNode) {
+
+ } elseif ($request instanceof \DOMNode) {
@Maks3w Collaborator
Maks3w added a note

Please import all this classes as was before.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((10 lines not shown))
* @return Server
*/
public function registerFaultException($class)
{
- $this->faultExceptions = array_merge($this->faultExceptions, (array) $class);
+ if (is_array($class)) {
+ foreach($class as $row) {
+ $this->registerFaultException($row);
+ }
+
+ } elseif (is_string($class) && class_exists($class)
+ && is_subclass_of($class, 'Exception')
+ ) {
+ $ref = new \ReflectionClass($class);
+
+ $this->faultExceptions[] = $ref->getName();
@Maks3w Collaborator
Maks3w added a note

Why $class is not enough here?

I don't understand your question. Are you asking why I am checking for type? Because, at least when I was writing this part there was no checking in Server::fault() where getMessage and getCode were used. And instead of thrown Exception user could only get error message about undefined methods. Because fault is public method anyone could fault by purpose in code. Later I added a check there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((33 lines not shown))
return $this;
}
/**
+ * Checks if provided fault name is registered as valid in this server.
+ *
+ * @param $fault Name of a fault class
+ *
+ * @return bool
+ */
+ public function isRegisteredAsFaultException($fault)
+ {
+ $ref = new \ReflectionClass($fault);
@Maks3w Collaborator
Maks3w added a note

The same as above note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Server.php
((9 lines not shown))
{
if ($fault instanceof \Exception) {
- $class = get_class($fault);
- if (in_array($class, $this->faultExceptions)) {
+ if ($this->isRegisteredAsFaultException($fault)) {
@Maks3w Collaborator
Maks3w added a note

:x: This not follows your own method signature. Expected string passed Exception instance

@param string|\Exception $fault ?? This is original method signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
((6 lines not shown))
*/
public function setClassMap($classMap)
{
$this->classMap = $classMap;
+ //@todo return Wsdl?
@Maks3w Collaborator
Maks3w added a note

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Soap/Wsdl.php
((28 lines not shown))
*/
- public function addTypes(DOMNode $types)
@Maks3w Collaborator
Maks3w added a note

All that param types are subsets of DOMNode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff
library/Zend/Soap/Wsdl.php
((8 lines not shown))
+ // remove namespace,
$pos = strrpos($type, '\\');
if ($pos) {
$type = substr($type, $pos+1);
@Maks3w Collaborator
Maks3w added a note

:x: This removes the base name or vendor name from the namespace but not all the namespace preffix. A\B > B but A\B\C > B\C

@Maks3w Collaborator
Maks3w added a note

I don't know why is this needed. /cc @weierophinney

strrpos gives you last occurrence of \ in string so substring of that +1 gives You last part of class. There is a test for that and I just added more cases to dataProvider and they all work. This is related to xml, names and portability but imho \ should become "."(dots)

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

What is doing this here?

Because of separate process it is not shown on the screen so I missed it. Fixed and committing.

Grzegorz Drozd Removed debug. 46c7507
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
((6 lines not shown))
* @return AutoDiscover
*/
public function setServiceName($serviceName)
{
+ $matches = array();
+ // first character must be letter or underscore {@see http://www.w3.org/TR/wsdl#_document-n}
+ $i = preg_match('/^[a-z\_]/ims', $serviceName, $matches);
@weierophinney Owner

Considering service names are single strings, why the "ms" regex modifiers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
((6 lines not shown))
* @return AutoDiscover
*/
public function setServiceName($serviceName)
{
+ $matches = array();
+ // first character must be letter or underscore {@see http://www.w3.org/TR/wsdl#_document-n}
+ $i = preg_match('/^[a-z\_]/ims', $serviceName, $matches);
+ if ($i != 1) {
+ throw new Exception\InvalidArgumentException('Service Name must start with letter or _');
+ }
@weierophinney Owner

The preg_match can be in the conditional:

if (!preg_match(/* ... */)) {
    throw new /*...*/;
}

That makes the intent more clear and easy to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
((37 lines not shown))
} else {
- throw new Exception\RuntimeException(
- "No service name given. Call Autodiscover::setServiceName()."
- );
+ throw new Exception\RuntimeException('No service name given. Call AutoDiscover::setServiceName().');
@weierophinney Owner

Revert this change; the line is too long now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
((8 lines not shown))
*/
public function setWsdlClass($wsdlClass)
{
- if (!is_string($wsdlClass) && !is_subclass_of($wsdlClass, 'Zend\Soap\Wsdl')) {
+ if (!is_string($wsdlClass) && !is_subclass_of($wsdlClass, '\Zend\Soap\Wsdl')) {
@weierophinney Owner

Revert this change. Class names inside strings are always considered globally qualified by PHP; the leading \\ is not necessary, and can actually occasionally lead to autoloader issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
((6 lines not shown))
* @return AutoDiscover
*/
public function addFunction($function)
{
- $this->functions[] = $function;
+ if (is_array($function)) {
+ foreach($function as $row){
+ $this->addFunction($row);
+ }
+ } elseif (is_string($function)) {
+ if (function_exists($function)) {
+ $this->functions[] = $function;
+ } else {
+ throw new Exception\InvalidArgumentException(
+ 'Argument to Zend\Soap\AutoDiscover::addFunction should be a valid function name.'
+ );
+ }
@weierophinney Owner

Rewrite to remove the need for an else block:

if (!function_exists($function)) {
    throw new Exception/*...*/;
}
$this->functions[] = $function;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
@@ -393,10 +450,11 @@ protected function _generateWsdl(array $reflectionMethods)
* Add a function to the WSDL document.
*
* @param $function \Zend\Server\Reflection\AbstractFunction function to add
- * @param $wsdl \Zend\Soap\Wsdl WSDL document
- * @param $port object wsdl:portType
- * @param $binding object wsdl:binding
+ * @param $wsdl \Zend\Soap\Wsdl WSDL document
+ * @param $port \DOMElement wsdl:portType
+ * @param $binding \DOMElement wsdl:binding
@weierophinney Owner

Either import these classes, or, if they are in the same namespace, use relative names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/AutoDiscover.php
@@ -414,7 +473,9 @@ protected function _addFunctionToWsdl($function, $wsdl, $port, $binding)
}
}
if ($prototype === null) {
- throw new Exception\InvalidArgumentException("No prototypes could be found for the '" . $function->getName() . "' function");
+ throw new Exception\InvalidArgumentException(
+ 'No prototypes could be found for the "' . $function->getName() . '" function'
+ );
@weierophinney Owner

Maybe use sprintf here to assemble the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
...over/DiscoveryStrategy/DiscoveryStrategyInterface.php
@@ -15,6 +15,7 @@
/**
* Describes how types, return values and method details are detected during AutoDiscovery of a WSDL.
+ *
@weierophinney Owner

No need for the extra line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
...utoDiscover/DiscoveryStrategy/ReflectionDiscovery.php
@@ -14,26 +14,58 @@
use Zend\Server\Reflection\ReflectionParameter;
/**
- * Describes how types, return values and method details are detected during AutoDiscovery of a WSDL.
+ * Describes how types, return values and method details are detected during
+ * AutoDiscovery of a WSDL.
+ *
@weierophinney Owner

No need for the extra line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
...utoDiscover/DiscoveryStrategy/ReflectionDiscovery.php
((8 lines not shown))
*/
class ReflectionDiscovery implements DiscoveryStrategyInterface
{
+ /**
+ * Returns description from phpdoc block
+ *
+ * @param \Zend\Server\Reflection\AbstractFunction $function
+ *
@weierophinney Owner

Please remove all extra lines between parameters/return/throws annotations; inconsistent with the rest of the framework, as we try to group these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
...utoDiscover/DiscoveryStrategy/ReflectionDiscovery.php
((19 lines not shown))
public function getFunctionDocumentation(AbstractFunction $function)
{
return $function->getDescription();
}
+ /**
+ * Return parameter type
+ *
+ * @param \Zend\Server\Reflection\ReflectionParameter $param
@weierophinney Owner

For all param, return, and throws annotations, use the imported class name or the name relative to the current namespace, and not the fully qualified class name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Client.php
((26 lines not shown))
+ protected $uri = null;
+ protected $location = null;
+ protected $style = null;
+ protected $use = null;
+ protected $login = null;
+ protected $password = null;
+ protected $proxyHost = null;
+ protected $proxyPort = null;
+ protected $proxyLogin = null;
+ protected $proxyPassword = null;
+ protected $localCert = null;
+ protected $passphrase = null;
+ protected $connectionTimeout = null;
+ protected $streamContext = null;
+ protected $userAgent = null;
+ /**#@-*/
@weierophinney Owner

Since you're re-flowing these anyways, let's alphabetize them, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Client.php
((33 lines not shown))
+ protected $proxyPort = null;
+ protected $proxyLogin = null;
+ protected $proxyPassword = null;
+ protected $localCert = null;
+ protected $passphrase = null;
+ protected $connectionTimeout = null;
+ protected $streamContext = null;
+ protected $userAgent = null;
+ /**#@-*/
+
+ /**#@+
+ * @var int
+ */
+ protected $cacheWsdl = null;
+ protected $features = null;
+ protected $compression = null;
@weierophinney Owner

Same comment here; alphabetize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Client.php
((6 lines not shown))
* @return Client
- * @throws Exception\ExceptionInterface with invalid uri argument
@weierophinney Owner

Typical order within ZF is param/return/throws. Additionally, we use a throws annotation per exception type thrown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Client/Common.php
((6 lines not shown))
if (extension_loaded('soap')) {
-class Common extends \SoapClient
+/**
+ */
@weierophinney Owner

Remove the empty docblock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Client/DotNet.php
((5 lines not shown))
*/
class DotNet extends SOAPClient
{
/**
+ * Curl HTTP client adapter.
+ * @var \Zend\Http\Client\Adapter\Curl
+ */
+ private $curlClient = null;
@weierophinney Owner

We tend to prefer protected visibility over private within ZF, unless there's an exceptional argument for using private.

@weierophinney Owner

Just realized you simply moved the property declarations; however, if you're refactoring, we might as well change the visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Client/DotNet.php
@@ -118,11 +153,13 @@ public function getLastResponseHeaders()
* Sets the cURL client to use.
*
* @param CurlClient $curlClient The cURL client.
- * @return self Fluent interface.
+ *
+ * @return DotNet Fluent interface.
@weierophinney Owner

phpDocumentor and most IDEs now support @return self for defining fluent interfaces.

Does @return self resolve to a class name of a extending class as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Exception/BadMethodCallException.php
@@ -9,7 +9,12 @@
namespace Zend\Soap\Exception;
+use BadMethodCallException as SPLBadMethodCallException;
+
+/**
+ * Exception thrown when method is badly called
@weierophinney Owner

Actually, the better description is "Exception thrown when invalid method name is called." The use case for BadMethodCallException is for throwing within a __call() method when a class does not define a virtual method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Exception/ExceptionInterface.php
@@ -9,5 +9,9 @@
namespace Zend\Soap\Exception;
+/**
+ * Common Exception interface
+ *
@weierophinney Owner

Remove the blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
...y/Zend/Soap/Exception/ExtensionNotLoadedException.php
@@ -9,5 +9,11 @@
namespace Zend\Soap\Exception;
+use RuntimeException;
+
+/**
+ * Exception thrown when soap php extension is not loaded
+ *
@weierophinney Owner

Remove the blank line (do this throughout).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((7 lines not shown))
use DOMDocument;
use DOMNode;
use SimpleXMLElement;
-use SoapFault;
-use stdClass;
-use Traversable;
+use ReflectionClass;
+use Zend\Soap\Exception;
+use Zend\Server\Server as ZendServerServer;
use Zend\Stdlib\ArrayUtils;
@weierophinney Owner

Keep these alphabetized, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
@@ -216,31 +235,35 @@ public function getOptions()
{
$options = array();
if (null !== $this->actor) {
- $options['actor'] = $this->actor;
+ $options['actor'] = $this->getActor();
}
@weierophinney Owner

Maybe use the shortcut ternary for these declarations?

$options['actor'] = $this->actor ?: $this->getActor();

This might aid readability throughout this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((10 lines not shown))
* @return Server
*/
public function setObject($object)
{
if (!is_object($object)) {
- throw new Exception\InvalidArgumentException('Invalid object argument ('.gettype($object).')');
+ throw new Exception\InvalidArgumentException('Invalid object argument (' . gettype($object) . ')');
@weierophinney Owner

Maybe use sprintf here to create the string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((17 lines not shown))
} elseif ($request instanceof SimpleXMLElement) {
$xml = $request->asXML();
+
@weierophinney Owner

I'm not convinced the spaces before each elseif are necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((30 lines not shown))
$dom = new DOMDocument();
- if (strlen($xml) == 0 || !$dom->loadXML($xml)) {
+ $loadStatus = $dom->loadXML($xml);
+
+ //@todo check libxml errors ? validate document ?
@weierophinney Owner

Add a space between the / and @.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((30 lines not shown))
$dom = new DOMDocument();
- if (strlen($xml) == 0 || !$dom->loadXML($xml)) {
+ $loadStatus = $dom->loadXML($xml);
@weierophinney Owner

loadXML() will raise an error if no XML is provided, which is why we originally checked to ensure we had something to pass to it first. I think this logic needs to be reinstated as it was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
@@ -774,7 +874,8 @@ protected function _getSoap()
* If no request is passed, pulls request using php:://input (for
* cross-platform compatibility purposes).
*
- * @param DOMDocument|DOMNode|SimpleXMLElement|stdClass|string $request Optional request
+ * @param \DOMDocument|\DOMNode|\SimpleXMLElement|\stdClass|string $request Optional request
@weierophinney Owner

Import these, if they are not already, and then remove the global qualifier from the class names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
@@ -798,11 +899,10 @@ public function handle($request = null)
$fault = false;
$this->response = '';
- if ($setRequestException instanceof \Exception) {
+ if (is_object($setRequestException) && is_subclass_of($setRequestException, 'Exception')) {
@weierophinney Owner

Restore the original; it does exactly the same thing, but is faster and more succinct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
@@ -819,12 +919,14 @@ public function handle($request = null)
// Send a fault, if we have one
if ($fault instanceof SoapFault && !$this->returnResponse) {
$soap->fault($fault->faultcode, $fault->getMessage());
+
@weierophinney Owner

Remove the blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((5 lines not shown))
return;
}
// Echo the response, if we're not returning it
if (!$this->returnResponse) {
echo $this->response;
+
return;
@weierophinney Owner

Remove the blank preceding line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((31 lines not shown))
return $this;
}
/**
+ * Checks if provided fault name is registered as valid in this server.
+ *
+ * @param $fault Name of a fault class
+ *
+ * @return bool
+ */
+ public function isRegisteredAsFaultException($fault)
+ {
+ $ref = new ReflectionClass($fault);
+
+ $classNames = $ref->getName();
+
+ return in_array($classNames, $this->faultExceptions);
@weierophinney Owner

Remove the blank lines; method is too short to make them necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
((9 lines not shown))
{
- if ($fault instanceof \Exception) {
- $class = get_class($fault);
- if (in_array($class, $this->faultExceptions)) {
+ if (is_object($fault) && is_subclass_of($fault, 'Exception')) {
@weierophinney Owner

Restore to:

if ($fault instanceof \Exception) {

As noted in previous comments, does the same thing more quickly and more succinctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
@@ -920,12 +1056,9 @@ public function fault($fault = null, $code = "Receiver")
$message = 'Unknown error';
}
- $allowedFaultModes = array(
- 'VersionMismatch', 'MustUnderstand', 'DataEncodingUnknown',
- 'Sender', 'Receiver', 'Server'
- );
+ $allowedFaultModes = array('VersionMismatch', 'MustUnderstand', 'DataEncodingUnknown', 'Sender', 'Receiver', 'Server');
@weierophinney Owner

Break into a multi-line declaration, one line per string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Server.php
@@ -939,8 +1072,9 @@ public function fault($fault = null, $code = "Receiver")
* @param string $errfile
* @param int $errline
* @param array $errcontext
+ * @throws \SoapFault
@weierophinney Owner

Remove global qualifier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl.php
((11 lines not shown))
+ */
+ const XML_NS = 'xmlns';
+ const XML_NS_URI = 'http://www.w3.org/2000/xmlns/';
+ const WSDL_NS = 'wsdl';
+ const WSDL_NS_URI = 'http://schemas.xmlsoap.org/wsdl/';
+ const SOAP_11_NS = 'soap';
+ const SOAP_11_NS_URI = 'http://schemas.xmlsoap.org/wsdl/soap/';
+ const SOAP_12_NS = 'soap12';
+ const SOAP_12_NS_URI = 'http://schemas.xmlsoap.org/wsdl/soap12/';
+ const SOAP_ENC_NS = 'soap-enc';
+ const SOAP_ENC_URI = 'http://schemas.xmlsoap.org/soap/encoding/';
+ const XSD_NS = 'xsd';
+ const XSD_NS_URI = 'http://www.w3.org/2001/XMLSchema';
+ const TYPES_NS = 'tns';
+ /**#@-*/
+
@weierophinney Owner

Constants should be declared before any properties; restore these to the original location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl.php
((9 lines not shown))
- * @return DOMElement
- */
- public function addSoapBinding(
- $binding,
- $style = 'document',
- $transport = 'http://schemas.xmlsoap.org/soap/http',
- $soapVersion = SOAP_1_1
- ) {
- $soapNs = $soapVersion == SOAP_1_1 ? self::SOAP_11_NS : self::SOAP_12_NS;
- $soapBinding = $this->dom->createElement($soapNs . ':binding');
+ * @param int $soapVersion SOAP version: SOAP_1_1 or SOAP_1_2, default: SOAP_1_1
+ *
+ * @return \DOMElement
+ */
+ public function addSoapBinding($binding, $style = 'document', $transport = 'http://schemas.xmlsoap.org/soap/http', $soapVersion = SOAP_1_1)
+ {
@weierophinney Owner

Restore the original formatting for the method declaration, as the line is getting quite long, and it becomes difficult to evaluate the number of arguments and their defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl.php
((44 lines not shown))
- $operation->setAttribute('name', $name);
+ * @param array|bool $input An array of attributes for the input element,
+ * allowed keys are: 'use', 'namespace', 'encodingStyle'.
+ * {@link http://www.w3.org/TR/wsdl#_soap:body More Information}
+ * @param array|bool $output An array of attributes for the output element,
+ * allowed keys are: 'use', 'namespace', 'encodingStyle'.
+ * {@link http://www.w3.org/TR/wsdl#_soap:body More Information}
+ * @param array|bool $fault An array with attributes for the fault element,
+ * allowed keys are: 'name', 'use', 'namespace', 'encodingStyle'.
+ * {@link http://www.w3.org/TR/wsdl#_soap:body More Information}
+ * @param int $soapVersion SOAP version: SOAP_1_1 or SOAP_1_2, default: SOAP_1_1
+ *
+ * @return \DOMElement The new Operation's XML_Tree_Node for use with {@link function addSoapOperation} and {@link function addDocumentation}
+ */
+ public function addBindingOperation($binding, $name, $input = false, $output = false, $fault = false, $soapVersion = SOAP_1_1)
+ {
@weierophinney Owner

Restore the original formatting for the method declaration, as the line is getting quite long, and it becomes difficult to evaluate the number of arguments and their defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl.php
@@ -552,31 +632,39 @@ public function getSchema()
*/
public function toXML()
{
+ $this->dom->normalizeDocument();
+
@weierophinney Owner

Remove empty line. Whenever a method is 5 lines or fewer, adding blank lines rarely aids significantly in readability. Fix this throughout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl.php
((40 lines not shown))
}
}
- return $elementXml;
+
+ return $elementXML;
+ }
+
+ /**
+ * Prepare attribute value for specific attributes
+ *
+ * @param string $name
+ * @param mixed $value
+ *
+ * @return string safe value or original $value
+ */
+ private function sanitizeAttributeValueByName($name, $value)
@weierophinney Owner

Use protected and not private visibility, unless there is an exceptional, compelling reason for private. If there is, you need to document why. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl/ComplexTypeStrategy/AnyType.php
@@ -19,19 +26,18 @@ class AnyType implements ComplexTypeStrategyInterface
*
* @param \Zend\Soap\Wsdl $context
*/
- public function setContext(\Zend\Soap\Wsdl $context)
- {
-
- }
+ public function setContext(Wsdl $context)
+ {}
@weierophinney Owner

Add one line break (but no empty line).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Soap/Wsdl/ComplexTypeStrategy/Composite.php
@@ -45,20 +43,22 @@ class Composite implements ComplexTypeStrategy
* @param array $typeMap
* @param string|ComplexTypeStrategy $defaultStrategy
*/
- public function __construct(array $typeMap=array(), $defaultStrategy='\Zend\Soap\Wsdl\ComplexTypeStrategy\DefaultComplexType')
+ public function __construct(array $typeMap=array(),$defaultStrategy = '\Zend\Soap\Wsdl\ComplexTypeStrategy\DefaultComplexType')
@weierophinney Owner

Add spaces around all = instances, and a space after the comma.

@weierophinney Owner

Also, remove the global qualifier from the ComplexTypeStrategy value.

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

@GrzegorzDrozd This looks like an EXCELLENT addition. I made a ton of comments, but there are some general ones I want to point out:

  • The accepted order for params/return/throws annotations is exactly that: params, return, throws. Additionally, there should be no empty lines between them.
  • You introduce a lot of empty lines that are largely unnecessary -- particularly when introduced between lines in a method or block with 5 lines or fewer to begin with. These can be removed.
  • We prefer protected visibility over private unless there are very compelling reasons for the stricter visibility. If you have such reasons, please document them.
  • $foo instanceof SomeClass is the same as is_object($foo) && is_subclass_of($foo, 'SomeClass') -- but is faster and more readable. :)
  • For classes referenced by docblock, the preferred practice is to use imports and/or names relative to the current namespace; add imports if none already exist. An import statement is a no-op if the code never references the class (and even a no-op in instanceof checks!), so they're cheap, and aid in readability. (Plus, phpdoc and most IDEs will expand based on imports and current namespace already!)

Finally, make sure you rebase, as master has diverged from when you created the original work.

Thanks in advance -- again, looks EXCELLENT!

@GrzegorzDrozd

Thank you for your appreciation :)

I'll fix the code and apply your comments. But due to lack of time, I might not finish before the weekend. Or even next weekend. :( I will try to do my best.

@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#3919] Incorporate feedback
- Ensure params/return/throws annotations are in correct order, with no
  whitespace between.
- Remove unnecessary empty lines.
- Ensure properties are in alphabetical order.
- Ensure classes referenced in docblocks are imported and/or resolved
  from current namespace.
2104fc7
@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#3919] Use instanceof
- instead of is_object() && is_subclass_of()
72b5d5f
@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#3919] CS fixes
- per php-cs-fixer
1eec4da
@weierophinney

Merged to develop for release with 2.2.0 -- thanks, @GrzegorzDrozd ! (I made the changes requested on merge)

@GrzegorzDrozd

Thank you! :) I have deadline on my project so I planned to take care of Your feedback during this, 5 day (at least in Poland ;) ) weekend. Sorry for extra work which You had to do.

ps. batch of next improvements coming soon :) This time smaller ;)

@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#3919] Incorporate feedback
- Ensure params/return/throws annotations are in correct order, with no
  whitespace between.
- Remove unnecessary empty lines.
- Ensure properties are in alphabetical order.
- Ensure classes referenced in docblocks are imported and/or resolved
  from current namespace.
50cd7ff
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#3919] Use instanceof
- instead of is_object() && is_subclass_of()
95b4475
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#3919] CS fixes
- per php-cs-fixer
e608fae
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'feature/3919' into develop
Close #3919
7e9b777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 11, 2012
  1. Changes in a way that WSDL is generated - dom and objects.

    Grzegorz Drozd authored
  2. More DOM

    Grzegorz Drozd authored
Commits on Jan 25, 2013
  1. Merge remote-tracking branch 'upstream/master'

    Grzegorz Drozd authored
    Conflicts:
    	library/Zend/Soap/Wsdl.php
    	tests/Zend/Soap/Server/DocumentLiteralWrapperTest.php
    	tests/Zend/Soap/WsdlTest.php
  2. Fixes in wsdl generation and tests.

    Grzegorz Drozd authored
Commits on Jan 29, 2013
  1. More code changes

    Grzegorz Drozd authored
Commits on Feb 24, 2013
  1. Old files removed

    Grzegorz Drozd authored
  2. Complete rewrite of wsdl generation.

    Grzegorz Drozd authored
    Complete rewrite of wsdl generation from strings and simple dom to full object oriented from the begining.
    It uses dom and proper namespaces. All tests are also rewritten to new structure and now are using xpath
    versus old string comparison.
  3. Merge branch 'master' of github.com:GrzegorzDrozd/zf2

    Grzegorz Drozd authored
    Conflicts:
    	library/Zend/Soap/Wsdl.php
    	tests/Zend/Soap/WsdlTest.php
  4. Quick fix in test.

    Grzegorz Drozd authored
    When runing clean from command line one test was failing. Fixed.
Commits on Feb 28, 2013
  1. Further improvements to the code.

    Grzegorz Drozd authored
    Code formating in accordance with Coding Style.
    More tests.
    Integration of recent code changes from ayastreb and weierophinney.
    Even more tests and test data.
  2. Missing tests for DotNet client.

    Grzegorz Drozd authored
  3. Merge branch 'master' of git://github.com/zendframework/zf2

    Grzegorz Drozd authored
    Fixed files permissions
  4. Another permission fix.

    Grzegorz Drozd authored
  5. First part of changes acording to code rewiev. Mostly style and forma…

    Grzegorz Drozd authored
    …ting changes.
  6. Final code reformat acording to code review.

    Grzegorz Drozd authored
  7. Damm you file permissions :/. Another fix.

    Grzegorz Drozd authored
  8. Fixed failing tests.

    Grzegorz Drozd authored
  9. Proper namespace constants.

    Grzegorz Drozd authored
    Change in namespace prefixes to use constants.
    Format and code style fixes acording to code review.
    Restpre AutoDiscovery::handle with tests.
Commits on Mar 1, 2013
  1. Restored missing function from AutoDiscover

    Grzegorz Drozd authored
    Code style and reformat
Commits on Mar 3, 2013
Commits on Mar 6, 2013
  1. Removed debug.

    Grzegorz Drozd authored
Something went wrong with that request. Please try again.