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

#416 - remove typehint in add_s3_signed_params_to_attachment_image_src #458

Closed
wants to merge 2 commits into from
Closed

Conversation

Koli14
Copy link

@Koli14 Koli14 commented Oct 26, 2020

This PR fix this issue: #416

Copy link

@hm-linter hm-linter bot left a comment

Choose a reason for hiding this comment

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

Linting failed (1014 errors).

(1013 notices occurred in your codebase, but were on files/lines not included in this PR.)

@joehoyle
Copy link
Member

I believe we fixed this in #401, which contains a bunch of refactoring. Just waiting on a review from @rmccue to push that forward.

@Koli14
Copy link
Author

Koli14 commented Oct 27, 2020

I believe we fixed this in #401, which contains a bunch of refactoring. Just waiting on a review from @rmccue to push that forward.

I don't see, if it's changed:

public function add_s3_signed_params_to_attachment_image_src( $image, int $post_id ) {

Although it's still might be fixed...

@joehoyle
Copy link
Member

Ahh ok good point, I thought that code had been refactored too, but perhaps not.

@@ -483,7 +483,7 @@ public function add_s3_signed_params_to_attachment_url( string $url, int $post_i
* @param integer $post_id
Copy link
Member

Choose a reason for hiding this comment

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

Could the phpdoc be updated too to reflect the possible types?

@joehoyle
Copy link
Member

joehoyle commented Dec 8, 2020

Hey @kodie I closed this in favour of #474, as there was a big amount of code churn from #401

@joehoyle joehoyle closed this Dec 8, 2020
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.

2 participants