- Issue created by @samit.310@gmail.com
- Issue was unassigned.
- Status changed to Needs review
over 1 year ago 4:10am 9 March 2023 - Status changed to Needs work
over 1 year ago 2:40pm 9 March 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
+ $module_dir = \Drupal::service('extension.list.module')->getPathname('entity_usage_light'); + $install_dir = $module_dir . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
Only a single space is necessary before and after the assignment operator. Since other dependencies have been injected, also that one should be.
/**
- * Returns the file url generator.
+ * Constructs an UsageController object.
*
- * @return \Drupal\Core\Extension\ModuleHandlerInterface
+ * @param \Drupal\Core\File\FileUrlGeneratorInterface $file_url_generator
+ * The file URL generator.
*/
- protected function fileUrlGenerator() {
- if (!$this->fileUrlGenerator) {
- $this->fileUrlGenerator = \Drupal::service('file_url_generator');
- }
- return $this->fileUrlGenerator;
+ public function __construct(FileUrlGeneratorInterface $file_url_generator) {
+ $this->fileUrlGenerator = $file_url_generator;
}The class name needs to be fully namespaced. Also, the article used for words starting with user is a.
+ * @param string $entity_type_id
+ * A entity type id.A is used for words that start with a consonant sound. id needs to be all uppercase letters.
+ * @return \|null * The bundle entity type or nothing.
\
is not a correct type name. - Assigned to imustakim
- Issue was unassigned.
- Status changed to Needs review
over 1 year ago 11:49am 10 March 2023 - Status changed to RTBC
over 1 year ago 5:10pm 28 March 2023 - 🇮🇳India saket-001
Applied the patch #5 its work fine remove all PHPCS error and changes are there as mentioned in #3
- Status changed to Needs work
over 1 year ago 8:12am 29 March 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
The patch could apply, but it is wrong.
I do not have time to make a full review, but at least the following change is not correct.
/** - * Returns the file url generator. + * Constructs a UsageController object. * - * @return \Drupal\Core\Extension\ModuleHandlerInterface + * @param \Drupal\Core\File\FileUrlGeneratorInterface $file_url_generator + * The file URL generator. */ - protected function fileUrlGenerator() { - if (!$this->fileUrlGenerator) { - $this->fileUrlGenerator = \Drupal::service('file_url_generator'); - } - return $this->fileUrlGenerator; + public function __construct(FileUrlGeneratorInterface $file_url_generator) { + $this->fileUrlGenerator = $file_url_generator; }
That is not the short description used for class constructors. It is also wrong, since it returns an instance for a different class.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
/** * @file + * Install file for Entity Usage Light. */
The comment for a .install file is usually Install, update, and uninstall hooks for the [module name] module. or Install, update, and uninstall hooks for [module name].
/** * Get media IDs out of a given entity's fields. * + * @param string $entity_type_id + * An entity type ID. * @param \Drupal\Core\Entity\EntityInterface $entity * A given entity. *
Since that comment is changed, the verb used in the short description should use the third person singular.
The articles used in the parameter descriptions are wrong./** * Check if the form is a Bundle configuration form. - * + * * @param \Drupal\Core\Form\FormStateInterface $form_state * The form state object. - * - * @return \..|null + * + * @return null * The bundle entity type or nothing. */
Since that comment is changed, the verb used in the short description should use the third person singular.
There should be just an empty line between the short description and the parameter descriptions or the return value description.
The type hint for the return value is wrong, given the description. If the method can returnsNULL
, the description must be changed too.- // Defaut values + // Defaut values. $bundle = $form_state->getFormObject()->getEntity();
That comment can be removed, since it does not say anything that is already clear reading the code.
'#description' => '<p>' . $this->t('Select the display in the usage tab per entity type.') . '</p>' . - '<p>' . $this->t('A default table will be used if there is no view available for a given entity type.') . '</p>', + '<p>' . $this->t('A default table will be used if there is no view available for a given entity type.') . '</p>',
The indentation is already correct.
- // Submit + // Submit. $form['actions']['submit']['#validate'][] = [$this, 'setThirdPartySettings'];
That comment can be removed too.
+ /** + * The plugin Cache Clearer. + * + * @var \Drupal\Core\Plugin\CachedDiscoveryClearer + */ + protected $pluginCacheClearer;
Words that are not at the beginning of a sentence are not written in capital case.
+ /** + * Private Route builder attribute. + * + * @var \Drupal\Core\Routing\RouteBuilderInterface + */ + private $routeBuilder;
Class properties are not described as attribute.
Words that are not at the beginning of a sentence are not written in capital case.+ // Clear enough caches to enable/disable "Usage" tab for selected + // entity types.
That comment does not make sense. To be correct, enough should be removed, caches should be written the cache, and two articles should be added.
- First commit to issue fork.
- 🇺🇸United States mortona2k Seattle
I added patch 5 to the issue fork. (The dev branch was an accident)
I was getting an error on the settings form and had to change CachedDiscoveryClearer to CachedDiscoveryClearerInterface because the service returns Drupal\Core\ProxyClass\Plugin\CachedDiscoveryClearer. I don't know if this is the correct fix.
- Status changed to Fixed
about 1 year ago 3:37pm 27 August 2023 Thank you very much for reporting and helping on this issue.
All errors should have been fixed on 1.0.3 now - except 2 warning on line length.
Marking as fixed.
Automatically closed - issue fixed for 2 weeks with no activity.