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

Reduce redundant else-branching (after throws), add missing @throws annotations #104

Merged
merged 4 commits into from
Sep 18, 2020
Merged

Conversation

kstenschke
Copy link

No description provided.

* @param string $method Is the method to execute
* @param mixed $param values for the method
* @param string $method Is the method to execute
* @param null $params
Copy link
Owner

@Shardj Shardj Sep 8, 2020

Choose a reason for hiding this comment

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

Params is mixed nullable with a default of null, not sure about source. I would put it as mixed or null|mixed

Copy link
Author

Choose a reason for hiding this comment

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

Though i agree that declaring nullability explicitly might be helpful, to my understanding, null is one of the types included in "mixed".
See: https://wiki.php.net/rfc/mixed-typehint

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough, feel free to change it to just mixed then

@kstenschke
Copy link
Author

kstenschke commented Sep 10, 2020

Was that finding accidental or after running some more thorough inspection?
if accidental, i will re-check the changeset of the PR, to ensure avoid introducing flaws.

Another thought: Do you consider tackling the suggestion from Issue #96?
if it is still feasible - i'd suspect the two forks having diverged already, might make synchronizing source from the two forks rather difficult. If #96 is considered: merging this PR would complicate that significantly, and should probably be delayed for now.

@Shardj
Copy link
Owner

Shardj commented Sep 18, 2020

I gave the change a quick skim through, so accidental. However once I found that one I did look for others and couldn't see any further problems

I'm not planning on merging with #96 at all

@Shardj Shardj merged commit 56cd7e4 into Shardj:master Sep 18, 2020
@kstenschke
Copy link
Author

Thanks for your feedback.
Wiith a less redundant code base, further enhancements should go easier, therefor i'd ask you to merge.
I also had a look at #96 / zfs, and think if they should decide on merging from zf1-future, also that shouldn't pose a problem.

@kstenschke
Copy link
Author

Thank you for merging 👍

@@ -161,6 +161,7 @@ public function __construct($mimetype)
* magic file.
*
* @return string
* @throws Zend_Validate_Exception
Copy link
Collaborator

Choose a reason for hiding this comment

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

this throws is bogus, it's never thrown (but caught and ignored silently)

will be removed when porting zf1s/zf1#54

Copy link
Collaborator

Choose a reason for hiding this comment

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

* @return array
* @throws Zend_Cloud_QueueService_Exception
*/
public function fetchQueueMetadata($queueId, $options = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this method also returns false.

@@ -348,7 +340,8 @@ public static function escape($keyword)
* @param string $strQuery
* @param string $encoding
* @return Zend_Search_Lucene_Search_Query
* @throws Zend_Search_Lucene_Search_QueryParserException
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zend_Search_Lucene_Search_QueryParserException is still thrown in parse()

@@ -709,8 +722,9 @@ public function getRequest()
/**
* Public access method to private Zend_Amf_Server_Response reference
*
* @param string|Zend_Amf_Server_Response $response
* @param string|Zend_Amf_Server_Response $response
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zend_Amf_Server_Response class does not exist:

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.

3 participants