- Issue created by @shashank5563
- 🇮🇳India shashank5563 New Delhi
headers' => [ 'Authorization' => 'Bearer ' . \Drupal::state()->get('api_calls_token'), 'X-API-Version' => 'next',
Use dependency injection for the state API.
- Status changed to Needs review
over 1 year ago 3:02pm 25 April 2023 - Status changed to RTBC
over 1 year ago 4:02pm 25 April 2023 - 🇵ðŸ‡Philippines kenyoOwen
Hi shashank5563
I applied your patch 3356250-3.patch → to the "Nginx Proxy Manager Connector" against Version 1.0.0-alpha1 and confirmed that the errors are resolved. Please see the screenshots attached.
For your review.
Thank you. - 🇮🇳India shashikanth171
@shashank5563 Please create a merge request, if possible.
- Status changed to Needs work
over 1 year ago 7:25am 27 April 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
* @param string $custom_domain + * Add custom domain. * @param string $custom_subdomain + * Add Sub Domain. * @param string $forward_host + * Add host. * @param string $port + * Add port number. * @param string $letsencrypt_email + * Add email for certification.
There is no need to start each description with Add.
- * @return array $response + * @return array + * It is returning the boolean.
That description is not correct. If it returns an array, it is not a Boolean value. (It is spelled Boolean, not boolean.)
* @param string $custom_domain + * Call custom domain. * @param int $proxy_host_id + * Define proxy host id.
Parameter descriptions do not start with Call not Define.
id is misspelled.* @return bool + * It is returning boolean
The type of the return value is already clear. The description must say what the purpose of the return value is.
- public function disable_proxy_host($custom_domain, $proxy_host_id = 0) { + public function disableProxyHost($custom_domain, $proxy_host_id = 0) { // Initiate API calls configuration. - if (!$this->api_call_init()) {
Changing the name of a public method is a disruptive change. It is better to leave that to a different issue.
+ * Use domain name to create proxy host. * @param string $forward_host + * Use host. * @param int $certificate_id + * Add certificate id for ssl. * @param string $port + * Add port number.
None of those descriptions make sense.
id and ssl are misspelled.- * @return \Drupal\Component\Serialization\Json $json + * @return \Drupal\Component\Serialization\Json + * Return the Json data.
The description for the return value does not start with Return.
- @shashank5563 opened merge request.
- @shashank5563 opened merge request.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
- * - * Use create_proxy_host() to add a new proxy_host - * - * Use deploy_proxy_host() to enable the proxy_host - * - * Use disable_proxy_host() to disable the proxy_host - * + * + * Use createProxyHost() to add a new proxy_host + * + * Use deployProxyHost() to enable the proxy_host + * + * Use disableProxyHost() to disable the proxy_host + *
Since that documentation comment is changed, each sentence must end with a period.
There is no need to add empty lines between each sentence.
proxy_host is misspelled.* @param string $custom_domain + * Custom domain. * @param string $custom_subdomain + * Sub Domain. * @param string $forward_host + * Forward host. * @param string $port + * Port number. * @param string $letsencrypt_email + * Mail for certification.
Each short description is missing an article.
- * @return array $response + * @return array + * It is returning the Array.
The short description must say what is returned, not its type, which is already clear from
@return array
.
Array is also misspelled.- public function create_proxy_host($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) { + public function createProxyHost($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) {
Changing names to public methods is disruptive. It is not anymore a matter of following the coding standards, but also not create issues for modules that are legitimately use public methods. These changes are rather to be considered in a separated issue.
+ * The action. * @param string $letsencrypt_email + * User email id for letsencrypt.
It would eventually be The user email [...], but is that really a user email?
for letsencrypt does not seem correct.* @param string $custom_domain + * Use the custom domain to get proxy host id.
Short descriptions for parameters do not start with Use.
id is misspelled, but it should not be necessary to say for which purpose is the custom domain used.* @return \Drupal\Component\Serialization\Json + * It is return the json data.
It is return is grammatically not correct. (It is not even a necessary phrase.)
json is misspelled.* @param string $domain_names + * Domain name to create proxy host. * @param string $forward_host + * Forward host. * @param int $certificate_id + * Certificate Id for SSL. * @param string $port + * Port number.
The parameter descriptions are each missing an article.
* @return bool + * It is return the Boolean.
The short description must say what it is returned, not its type, which has been already said.
- Status changed to Needs review
over 1 year ago 7:36am 2 May 2023 - 🇮🇳India shashank5563 New Delhi
@shashikanth171, I have updated the MR (fixcodingstandard-3356250 ). Please review and merge.
- Status changed to Needs work
over 1 year ago 8:14am 2 May 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
* @param string $custom_domain + * An Custom domain. * @param string $custom_subdomain + * An Sub Domain. * @param string $forward_host + * An Forward host. * @param string $port + * An Port number.
I did not say which article should be used. An article means each line is missing a single article, not two or more articles.
An is not used when the following word starts with a consonant sound.* @param string $letsencrypt_email + * Mail for certification.
That description is missing an article.
Is that description correct?+ * @return array + * */ - public function create_proxy_host($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) { + public function createProxyHost($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) {
The return value description is missing.
- public function deploy_proxy_host($custom_domain, $proxy_host_id = 0) { + public function deployProxyHost($custom_domain, $proxy_host_id = 0) {
Changing names to public methods is disruptive. It is not anymore a matter of following the coding standards, but also not create issues for modules that legitimately use public methods. These changes are rather to be considered in a separated issue.
* @param string $old_custom_domain + * An Custom domain. * @param string $new_custom_domain + * An Custom domain.
The same description is repeated for two different parameters. Since the parameters are different, the descriptions should be different.
* @return bool + *
The return value description is missing. That is what
phpcs
is reporting: Description for the @return value is missing.* @param string $custom_domain + * The custom domain to get proxy host name.
The description is missing an article. I am not sure it is necessary to say for which purpose the custom domain is used.
* @param string $custom_domain + * The custom domain to get certificate id.
The description is missing an article.
id is misspelled, since it is ID. - Status changed to Needs review
over 1 year ago 9:45am 2 May 2023 - 🇮🇳India shashikanth171
@apaderno Are you suggesting that we can modify some functions from public to protected or private?
Changing names to public methods is disruptive. It is not anymore a matter of following the coding standards, but also not create issues for modules that are legitimately use public methods. These changes are rather to be considered in a separated issue.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
No, I am saying that the name of public methods cannot be simply changed because
phpcs
says to do so. That is a change for a different issue, which eventually must be done in a different branch. - 🇮🇳India shashikanth171
@apaderno, I am changing the issue's scope to cover all Drupal coding standard related items.
@shashank5563, please go through all code comments and correct any issues you may find.
- 🇮🇳India shashank5563 New Delhi
@apaderno, I have updated the MR (fixcodingstandard-3356250 ). Please review and merge.
-
shashikanth171 →
committed 4e8b1a62 on 1.x authored by
shashank5563 →
Issue #3356250 by shashank5563, shashikanth171, apaderno: Comply with...
-
shashikanth171 →
committed 4e8b1a62 on 1.x authored by
shashank5563 →
- Status changed to Fixed
over 1 year ago 12:35pm 3 May 2023 - 🇮🇳India shashikanth171
@shashank5563 Made some changes to the comments in the MR. Merged the MR and marking the issue Fixed.
- Status changed to Fixed
over 1 year ago 11:55am 8 May 2023