- Issue created by @Harshita mehra
- Status changed to Needs review
over 1 year ago 1:13pm 28 June 2023 - 🇮🇳India Harshita mehra
I have provided a patch to fix this issue.
Please review it. - Status changed to RTBC
over 1 year ago 1:10pm 29 June 2023 - 🇮🇳India Raveen Kumar
Hello Harshita,
I have reviewed & implemented the patch for the custom account link module version 2.x dev.
The patch has been applied successfully as you can see in my attached screenshots.
Please have a look. And Thank You. - Status changed to Needs work
over 1 year ago 7:57am 20 July 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
The issue summary should always describe what the issue is trying to fix and, in the case of coding standards issues, show which command and arguments have been used and which report that command shown. In this way, project maintainers can verify the patch/MR fixes all the warnings/errors.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Then, the patch could apply, but that does not mean the changes done are correct.
+/** + * Custom + * + * Custom account link settings. + * + * @category Components + * @package Drupal + * @author Your Name <yourname@example.com> + * @license https://www.gnu.org/licenses/gpl-3.0.txt GNU/GPLv3 + * @link https://yoursite.com + */
Those tags are not used in Drupal modules.
The license is not even correct, since Drupal modules / themes / installation profiles hosted on drupal.org are licensed under the same license used for Drupal core, which is not GPLv3.
@author
and@link
are using placeholder values. Either you use the correct values, or those tags can be removed (but see the first sentence).+ /** + * Class CustomAccountLinkConfigForm. + * + * @category Image + * @package Drupalimage_Delta_FormatterForm + * @author author <author@test.com> + * @license http://opensource.org/licenses/gpl-license.php GNU Public License + * @link http://example.com + */ +class CustomAccountLinkSettings extends ConfigFormBase
The description is still just repeating the class name. It must describe the class purpose.
Those tags are not used in Drupal code.
Once again, the license is wrong.+ /** + * {@inheritdoc} + * + * @return string + */ + protected function getEditableConfigNames()
SInce that is an inherited method (from a parent class or an interface), there is no need to use
@return
. The existing code is already correct.- /** - * {@inheritdoc} - */ - public function buildForm(array $form, FormStateInterface $form_state) { - $config = $this->config('custom_account_link.settings'); + /** + * {@inheritdoc} + * + * @param array $form + * The complete form array. + * @param \Drupal\Core\Form\FormStateInterface $form_state + * The current state of the form. + * + * @return string + */ + public function buildForm(array $form, FormStateInterface $form_state) + {
The patch is changing code that is already correct. Also, the opening curly parenthesis does not go on a new line, as per Drupal coding standards.
+ public function buildForm(array $form, FormStateInterface $form_state) + { + $config = $this->config('custom_account_link.settings');
It seems the last line is not correctly indented by one space.
+/** + * Custom + * + * Registration menu link. + * + * @category Components + * @package Drupal + * @author Your Name <yourname@example.com> + * @license https://www.gnu.org/licenses/gpl-3.0.txt GNU/GPLv3 + * @link https://yoursite.com + */
Drupal does not use that documentation comment, nor those tags.
-class RegisterMenuLink extends MenuLinkDefault { +class RegisterMenuLink extends MenuLinkDefault +{
See PHP coding standards → , which explain why the change is not correct.
- /** - * Constructs a new LoginLogoutMenuLink. - * - * @param array $configuration - * A configuration array containing information about the plugin instance. - * @param string $plugin_id - * The plugin_id for the plugin instance. - * @param mixed $plugin_definition - * The plugin implementation definition. - * @param \Drupal\Core\Menu\StaticMenuLinkOverridesInterface $static_override - * The static override storage. - * @param \Drupal\Core\Session\AccountInterface $current_user - * The current user. - */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, StaticMenuLinkOverridesInterface $static_override, AccountInterface $current_user) { - parent::__construct($configuration, $plugin_id, $plugin_definition, $static_override); + /** + * Constructs a new LoginLogoutMenuLink. + * + * @param array $configuration + * A configuration array containing information about the plugin instance. + * @param string $plugin_id + * The plugin_id for the plugin instance. + * @param mixed $plugin_definition + * The plugin implementation definition. + * @param \Drupal\Core\Menu\StaticMenuLinkOverridesInterface $static_override + * The static override storage. + * @param \Drupal\Core\Session\AccountInterface $current_user + * The current user. + */
The existing code is already correct as it is.
- 🇮🇳India Harshita mehra
@apaderno, I'm working on it.
Thanks for your review and for providing the correct way. - 🇮🇹Italy apaderno Brescia, 🇮🇹
The status is also for the issue summary which needs to be updated as per my previous comment. Providing a new patch/MR is not sufficient to change the issue status.