[1.0.x] DonorPerfect

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

1. Fix phpcs issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml donorperfect/

FILE: donorperfect/modules/gift/src/Form/GiftForm.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 51 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/modules/other/src/Form/OtherForm.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 51 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/modules/contact/src/Form/ContactForm.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 51 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/modules/donor/src/Form/DonorForm.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 51 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/modules/donor/src/Entity/DonorController.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 71 | ERROR | The array declaration extends to column 97 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/modules/donor/src/DonorNameAlphaFieldItemList.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 10 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
-------------------------------------------------------------------------------

FILE: donorperfect/donorperfect.routing.yml
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 32 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

FILE: donorperfect/donorperfect.module
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 9 | WARNING | Format should be "* Implements hook_foo().", "* Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements hook_foo_BAR_ID_bar() for
   |         | xyz-bar.html.twig.", "* Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.", or "* Implements hook_foo_BAR_ID_bar() for block templates."
-------------------------------------------------------------------------------

FILE: donorperfect/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 7 WARNINGS AFFECTING 7 LINES
----------------------------------------------------------------------
 18 | WARNING | Line exceeds 80 characters; contains 81 characters
 25 | WARNING | Line exceeds 80 characters; contains 83 characters
 37 | WARNING | Line exceeds 80 characters; contains 81 characters
 57 | WARNING | Line exceeds 80 characters; contains 81 characters
 58 | WARNING | Line exceeds 80 characters; contains 82 characters
 59 | WARNING | Line exceeds 80 characters; contains 81 characters
 70 | WARNING | Line exceeds 80 characters; contains 81 characters
----------------------------------------------------------------------

FILE: donorperfect/src/DPQuery.php
-------------------------------------------------------------------------------
FOUND 33 ERRORS AND 9 WARNINGS AFFECTING 42 LINES
-------------------------------------------------------------------------------
    9 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
   64 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  103 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  118 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  126 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  169 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  226 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  287 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  323 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  329 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  376 | ERROR   | The array declaration extends to column 97 (the limit is 80). The array content should be split up over multiple lines
  409 | ERROR   | The array declaration extends to column 100 (the limit is 80). The array content should be split up over multiple lines
  443 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  526 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  545 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  564 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  641 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  723 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  744 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
  970 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1060 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1142 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1186 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 1239 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1618 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1791 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1847 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1953 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1969 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 1985 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 2017 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 2027 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 2030 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 2031 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 2125 | ERROR   | The array declaration extends to column 90 (the limit is 80). The array content should be split up over multiple lines
 2213 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 2245 | ERROR   | The array declaration extends to column 102 (the limit is 80). The array content should be split up over multiple lines
 2302 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 2310 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 2343 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 2351 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 2368 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/src/Plugin/views/query/DonorPerfect.php
-------------------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 7 LINES
-------------------------------------------------------------------------------
  34 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
  84 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 142 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 216 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 244 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 258 | ERROR | The array declaration extends to column 85 (the limit is 80). The array content should be split up over multiple lines
 334 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
-------------------------------------------------------------------------------

FILE: donorperfect/src/Form/EntitySettingsFormBase.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------------
  50 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 126 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 127 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/src/Form/SettingsForm.php
-------------------------------------------------------------------------------
FOUND 3 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 82 | ERROR   | The array declaration extends to column 108 (the limit is 80). The array content should be split up over multiple lines
 82 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 82 | ERROR   | The array declaration extends to column 107 (the limit is 80). The array content should be split up over multiple lines
 82 | ERROR   | The array declaration extends to column 106 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/src/Entity/Query/QueryAggregate.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 108 | ERROR | The array declaration extends to column 110 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/src/Entity/ControllerInterface.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 14 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
-------------------------------------------------------------------------------

FILE: donorperfect/src/Entity/ControllerServiceInterface.php
-------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-------------------------------------------------------------------------------
 24 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 33 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 42 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 56 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 67 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 78 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
-------------------------------------------------------------------------------

FILE: donorperfect/src/Entity/ControllerService.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
-------------------------------------------------------------------------------
 30 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 67 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------

FILE: donorperfect/src/Entity/DefaultTableMapping.php
-------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------
  76 | ERROR | The array declaration extends to column 107 (the limit is 80). The array content should be split up over multiple lines
 146 | ERROR | The array declaration extends to column 81 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/src/DPUtility.php
-------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
-------------------------------------------------------------------------------
 194 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 245 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 287 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 418 | ERROR | The array declaration extends to column 94 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/src/Element/Name.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 174 | ERROR | The array declaration extends to column 104 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/src/Element/Email.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 71 | ERROR | The array declaration extends to column 106 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------

FILE: donorperfect/src/Controller/DonorPerfectController.php
-------------------------------------------------------------------------------
FOUND 4 ERRORS AND 1 WARNING AFFECTING 5 LINES
-------------------------------------------------------------------------------
  55 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
  84 | ERROR   | [x] Namespaced classes/interfaces/traits should be referenced with use statements
  95 | ERROR   | [ ] The array declaration extends to column 89 (the limit is 80). The array content should be split up over multiple lines
 126 | ERROR   | [ ] The array declaration extends to column 92 (the limit is 80). The array content should be split up over multiple lines
 145 | ERROR   | [ ] The array declaration extends to column 93 (the limit is 80). The array content should be split up over multiple lines
-------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------

FILE: donorperfect/donorperfect.api.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 29 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
-------------------------------------------------------------------------------

2. FILE: donorperfect.module

/**
 * @file
 * Code for the DonorPerfect module.
 */

The usual description for a .module file is Hook implementations for the [module name] module. where [module name] is the module name given in the .info.yml file.

Hello @vishal.kadam, thanks a lot for the feedback. I think all the corrections have been made.

Rest looks fine to me.

Let’s wait for a Code Review Administrator to take a look and if everything goes fine, you will get the role.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or does not correctly use the Drupal API; the single points are not ordered, not even by importance

src/Controller/ControllerBase.php

The parent class already defines container() and it is not necessary to define configFactory(), since there is already config() in the parent class.

src/Controller/DonorPerfectController.php

      $request = $this->container()->get('request_stack')->getCurrentRequest();
      foreach (['element_token', 'donor_type', 'last_name', 'first_name'] as $request_key) {
        $request_values[$request_key] = $request->get($request_key);
      }

Dependencies must be added using the create() method, like done from the AdminController class, the ContactController class, or the ShortcutController class, which all extend the ControllerBase class.

      $result_content[] = ['#markup' => Markup::create('<h3>Possible Matches in DonorPerfect</h3>')];
      $caption = (count($donors) == 1) ? '1 possible match' : count($donors) . ' possible matches';

The Markup class is marked @internal. It must not be used by contributed modules, and there should not be any reason to use it, since Drupal core allows to use a render array or a string containing markup.
Strings shown in the user interface must be translatable. Drupal core has also a class/method for strings that depend on a dynamic value (a string when the value is singular; a string when the value is plural).

$link = Markup::create('<a href="#" result-data=' . str_replace(' ', '&nbsp;', Json::encode($donor_data)) . '>' . $this->t('Match') . '</a>');

There is a class to generate links.

src/Entity/Storage.php

        if ($field_name === 'flag') {
          $sql = "SELECT donor_id AS matching_id, flag AS code from dpflags WHERE donor_id IN (" . implode(',', $ids) . ")";
        }
        else {
          $sql = "SELECT matching_id, code FROM dpusermultivalues WHERE (matching_id IN (" . implode(',', $ids) . ")) AND (field_name='" . strtoupper($field_name) . "')";
        }

SQL queries are not built concatenating strings. Placeholders must be used.

src/Entity/Query/Condition.php

Drupal\Core\Entity\Query\Sql\Condition is not part of the public API and cannot be extended from contributed modules. Drupal\Core\Entity\Query\ConditionBase is a base class and can be extended.

src/Entity/Query/QueryFactory.php

The Drupal\Core\Entity\Query\Sql\QueryFactory is not part of the public API too. In this case, it does not even make sense to extend a class and not change any defined method.

src/DPQuery.php

ControllerBase is a class for controllers. It cannot be used for a generic class that is not used for controllers.

src/DPUtility.php

If a class uses \Drupal's methods, then it should probably be converted to a service class.

Hello @apaderno, thanks so much for your review of my module. I believe all the issues you identified have now been corrected, except for one point about which I'd like to ask for more clarification.

You mentioned that some of the classes I extended are not part of the public API, and therefore cannot be extended by contributed modules. I saw in this comment → on another application that you said that only classes marked with the @api tag and classes named 'Base' are part of the public API. However, when I searched the code for Drupal 10 core, I did not find any classes marked with the @api tag. Would you mind providing some examples of classes with this tag? Or would you mind providing a link to some documentation that explains which classes can and cannot be extended so that I can understand better?

My understanding has been that core classes that should not be extended are the ones marked with the @internal tag, or classes that are programmed as 'final class' (for example, \Drupal\Core\Config\ManagedStorage), but it is very possible I might be wrong about this.

Thank you again for your time.

No, the absence of the @internal tag does not mean the class is public API.
Although I have summarized what can be considered public API (what is marked with @api or base classes), that is essentially correct.
In fact, not even the public or protected class properties are public API. In the latter case, the exception is protected properties on a base class marked with @api, or properties that are accessed with __get() or __set() (for the classes that implement those magic methods). That means, for example that I could use $object['name_of_a_property_that_really_is_protected'], where $object is an instance of a class implementing those magi methods, without to worry in the case the code returns the value of a protected property.

The only method that Drupal 10 marks with @api is ConfigEntityUpdater::update().

You found the same thing I did, that there is only one method in Drupal core that is tagged with @api (ConfigEntityUpdater::update()), but there are no classes with this tag. This leads me to believe that an @api tag is not the way Drupal core identifies which classes can or cannot be extended. In that case, does this mean that only classes with the word 'Base' can be extended?

I have been searching for documentation that explains which core classes are allowed to be extended by contributed modules, but I haven't found it yet. But in my search, I came across this issue → for core where there is a lot of discussion by core developers about contributed modules extending the Node class, yet no one says that the Node class cannot be extended by contributed modules because it is not part of the public API. What is it about the Node class that identifies it as being part of the public API as opposed to the classes extended in my project?

The reason I extended the classes I did is to avoid a lot of code duplication. Those classes already do a majority of the work, but I just needed to tweak their behavior in a few ways. This is the beauty of OO programming in general and of Drupal in particular. The fact that Drupal's classes are developed in such a way that contributed modules can override almost any aspect of its behavior is a true testament to its genius and one of the main reasons I love working with it.

If you know of some documentation that clearly explains which core classes are allowed to be extended and which are not, I would truly appreciate if you could give me a link so that I can understand this point more clearly.

I really do appreciate all the time you dedicate to working with these applications for the security coverage role. Thank you again.

@brandonpost
https://www.drupal.org/about/core/policies/core-change-policies/bc-policy → Is the backend “API” policy.

Short version: very little of Drupal’s code is actually considered API, a significant portion of code is @internal by default. Even for code(interfaces) that is API in one scenario(caller) may not be API in another scenario (implementer).

@cmlara, thank you so much for that. That is exactly the documentation I was searching for but couldn't find for some reason (I guess I wasn't using the right search terms). I see now the documentation for the @api tag that @apaderno was referring to, which I hadn't been able to find documented before.

I think the most relevant part of this documentation for the current discussion is this section: →

# PHP and JavaScript classes
In general, only interfaces can be relied on as the public API.
With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do. (emphasis added)

I believe the last caveat of that statement is where my module falls, and I am prepared to update my code if anything changes within the classes I have extended.

The thing about this module is that it is communicating extensively with a remote SQL database. However, it does not have direct access to that database, but rather must submit query strings to DonorPerfect's API and receive the results returned from the API. This issue about query strings is something that @apaderno caught in his review, saying, "SQL queries are not built concatenating strings. Placeholders must be used." In a normal situation, where the module has direct access to the database, I agree completely with this statement, as building SQL queries through string concatenation is an absolute security threat vector. But in this case, where query strings is the only means this module has to access the database, using string concatenation becomes a necessary evil. It means that the strings being concatenated must be thoroughly validated, which this module tries to do.

Back to the point about extending core classes, the core developers have done so much amazing work building tools to compile SQL queries. All of those tools are extremely useful for this module since it is communicating with a SQL database, but it just has to make a few modifications so that the result is a compiled query string that can be submitted to DonorPerfect's API rather than directly to a database server. This is the reason I have extended core's SQL classes rather than duplicating all that code, and I am prepared to update my code if there are changes to core's SQL classes in the future.

I hope this provides more clarity about what this module is doing, and why I took the approach I did. Thank you.

I believe the last caveat of that statement is where my module falls, and I am prepared to update my code if anything changes within the core classes I have extended.

Except that the quoted part applies when nothing else in the page applies.
If only interfaces can be relied on as the public API, then I could not access a protected property via __get(), nor set it via __set(), since those methods are not defined in an interface, contrary to what the following sentence says.

Public properties on a class are always considered @internal. This does not include the use of __get() and __set() to provide access to protected properties.

The part you quote does not even limit what said in other parts of the page. It is not that controller classes can be used as parent class only when they implement an interface; they cannot used as parent class in any case. Similarly, it is not that classes that implement a plugin can be used as parent class when they implement an interface; they cannot be used as parent class in any case.

If the PHP and JavaScript classes part were always applicable, other parts in that page should not be present.

• Protected properties: Interfaces do not define properties; there is no need to say they are internal API because that is covered from only interfaces can be relied on as the public API
• Public properties: Interfaces do not define properties; there is no need to say they are internal API for the same reason.
• Public methods not specified in an interface: Those methods are not covered by an interface; there is no need to say they are internal API when it is already said I cannot rely on them.

The reason I extended the classes I did is to avoid a lot of code duplication.

The class in src/Controller/ControllerBase.php contained code that was not necessary, and the class in src/Controller/DonorPerfectController.php still contains code that is not necessary, especially because it defines a property that is already defined from the parent class. That is duplication too.

Apart from the parent classes, there are other changes that have not been done.

@apaderno, I'm sorry, we seem to be having a miscommunication that I hope we can clear up.

In comment #13, I quoted the # PHP and JavaScript Classes section from the documentation. In comment #14, you seem to focus on the first line of that section which says, "In general, only interfaces can be relied on as the public API." Please know that I understand and agree with everything you said in comment #14. The point I was trying to make in comment #13 was not related to that first line about interfaces at all. After that first line, it goes on to say that core classes might change, and therefore developers "should not inherit from classes in the system, or be prepared to update code for changes if they do." This does not seem to be setting a hard line rule that core classes can never be extended in any case. Rather, it seems to be saying that, in general, core classes should not be extended, but if a developer does extend a core class, then that developer should be prepared to update his code if the core class changes.

In comment #13, I presented my reasoning as to why I extended the core SQL classes, and I stated that I understand and am prepared to update my code in the event that the core classes change in the future. I believe this aligns with what the documentation says about extending core classes. But if you do not agree that my reasoning justifies extending the core classes in this case, please let me know and I can work from there.

Moving on to comment #15:

The class in src/Controller/ControllerBase.php contained code that was not necessary;

I believe you are aware that this file no longer exists, but it seems you are mentioning it to make a point that I have duplicated code, even after I said in comment #13 that I extended the core classes in order to avoid duplicating code. Please let me explain. In comment #8, you said that my ControllerBase class contained a container() method that was already in the parent class, and was therefore code duplication. However, in the parent class the container() method was private, whereas I overrode it to make it protected, so that it could be accessed by my subclasses, and therefore it was not actually code duplication. You also said that my configFactory() method was unnecessary since the parent class already had a config() method. However, the config() method in the parent class only provides immutable config, but I had a subclass extending my ControllerBase that required editable config, and thus the need for the configFactory() method. Therefore, I disagree that my ControllerBase class contained any duplicated code. However, based on your feedback, I refactored my code to do away with the ControllerBase class completely, and I think the module is better now because of that, so I thank you for that.

the class in src/Controller/DonorPerfectController.php still contains code that is not necessary, especially because it defines a property that is already defined from the parent class. That is duplication too.

Here again it seems you are making a point that I have duplicated code after I said that I extended the core classes in order to avoid code duplication. But again I disagree that there is any code duplication here. I believe the property you are referring to is the $container property. But the parent class actually does not have a $container property. It does have a container() method, but that method is private, making it inaccessible to my subclass. Therefore, I added a $container property so that it could be used within my subclass.

I truly do want to avoid code duplication, so please let me know if you have found any other instances, and I will work to resolve it.

Moving to comment #16 where you said there are other changes that have not been done, I have reviewed each point you made in comment #8, and I have tried to make all the appropriate changes in my code. I understand that your points in #8 were examples and that there were other places in my code where your points applied, and I have tried to correct all of those as well. The git history should show that I made significant changes to my code after your review, and I believe it is a much better module now thanks to your guidance. If there are changes that still need to be done, please give me some direction as to what they are, and I will work on them. Thank you again.

I quoted the # PHP and JavaScript Classes section from the documentation. In comment #14, you seem to focus on the first line of that section which says, "In general, only interfaces can be relied on as the public API."

No, I focus on the fact that part applies when other parts do not apply. Otherwise, that full documentation page would be much shorter: It would need to just contain the PHP and JavaScript Classes section, instead of having all those parts about classes, interfaces, and properties.

Since the purpose of that section does not seem clear, let me ask this: Are there classes implemented by Drupal core or Drupal core modules, which are not usually used as parent class from classes implemented by contributed modules, for which the constructor is not called (not even from Drupal core), and for which none of the properties are used by a contributed module?

I believe you are aware that this file no longer exists, but it seems you are mentioning it to make a point that I have duplicated code

If the aim is not duplicating code, creating a class just to duplicate a method should not be done. Yes, ControllerBase::container() is private, but that does not mean that overring that method just to make it protected or public is not duplicating code.
Furthermore, the reason for which that method is private is documented in that method.

This method is marked private to prevent sub-classes from retrieving services from the container through it. Instead, \Drupal\Core\DependencyInjection\ContainerInjectionInterface should be used for injecting services.

That is exactly the opposite of what DonorPerfectController did. I have reported that in comment #8 → . I reviewed the code further two times, to discover there are still changes that need to be done. (Hint: Needs review is not for letting reviewers answer any question applicants have; on this queue, that status means I have changed the code as requested and the project is ready for another review.)

src/Entity/ControllerService.php

Services do not need a property containing the dependency container: Each dependency a service has is defined in the service definition. The class is also using the container for the wrong purpose: Forms are instantiated using the form_builder service.

src/Entity/Storage.php

      $entity = new $entity_class($entity_values, $this->entityTypeId);
      $entities[$entity_id] = $entity;

Entity objects are created calling the create() method.

  /**
   * Gets the DPQuery service.
   *
   * Using \Drupal::service() here, following the example set in this class's
   * parent class \Drupal\Core\Entity\EntityHandlerBase::moduleHandler().
   *
   * @return \Drupal\donorperfect\DPQuery
   *   The DPQuery service.
   */
  protected function dpQuery() {
    if (!$this->dpQuery) {
      $this->dpQuery = \Drupal::service('donorperfect.dpquery');
    }
    return $this->dpQuery;
  }

As a side note, it is correct to use \Drupal methods in that case, but not because Drupal\Core\Entity\EntityHandlerBase::moduleHandler() does it. It is because classes that implement Drupal\Core\Entity\EntityStorageInterface cannot also implement Drupal\Core\DependencyInjection\ContainerInjectionInterface. The only way they have to get dependency injected is using \Drupal methods.

src/Form/EntitySettingsFormBase.php

$entity_type_label = ucfirst($this->getEntityType()->getSingularLabel());

For a label that starts with a capital letter, there is getLabel(). getSingularLabel() takes the value returned from getLabel(), passes it to mb_strtolower(), and then this code passes that value to ucfirst(). It is not necessary, since getLabel() returns strings like 'Content', 'User', 'Taxonomy term', or 'Taxonomy vocabulary' (just to make a few examples with Drupal core entities).

src/FormValidator.php

  public static function filterCheckPlain(string $input) {
    if (!empty($string)) {
      return Html::escape($input);
    }
    return '';
  }

Just to point out that method will always return an empty string.

No, I focus on the fact that part applies when other parts do not apply. Otherwise, that full documentation page would be much shorter: It would need to just contain the PHP and JavaScript Classes section, instead of having all those parts about classes, interfaces, and properties.

Respectfully I disagree, the section must be used in context of the other sections, since the entire page is about PHP classes this section acts as a general clarification that is binding over the entire page unless explicitly revoked.

Addtionaly the majority of that page is actually clarification for Core Committers (and core contributors) to understand the BC policy rather than direction regarding the use of Drupal's methods.

There are many should not, and no must not phrases listed in the BC policy page.

OOP standards indicate that if the Drupal Core Team is to list as a class to never be extended it MUST be final and for a property to never be accessed it must be private. Lacking final/private the it may not be recommended to do so however the extender is free to accept that risk.

The Drupal Core team has a history of not using final or private in order to allow code to be extended even if it is off API (see SLACK #contribute there were recent discussions on this regarding final and Access Policies). Furthermore the Drupal Core Team routinely and intentionally goes out of its way to maintain BC with non API classes, methods and properties (to the detriment of Cores code base) under the assumption that non API classes will be used directly.

Extending a non-final class is not on its own a sign of insecure programming or misuse of the Drupal API, only that the program has accepted additional support burden.

More importantly, IIRC this queue has approved applications extending non API classes in the past year.

That said to the applicant:
My suggestion is to not extend core classes, and do not use ServiceContollerBase. Extending classes will just make life more painful and tie your code to specific patch releases of core while ServiceControllerBase breaches the standard of Dependency Injection. Both of these make your code brittle and harder to test. Code duplication is not always bad.

I have not reviewed the code in detail, this message is solely in response to @apaderno's assertions regarding the BC policy and its impact on Code Review.

I have changed the code as requested and the project is ready for another review.

@cmlara, thanks a lot for the advice. I followed it in this case, and it will continue to be helpful moving forward.

I am changing priority as per Issue priorities → .

Respectfully I disagree, the section must be used in context of the other sections

Exactly, but that means that if a section says that the parent class can only be a base class, what follows must be read considering that part.

As for that policy, it clearly states:

This document defines what parts of the Drupal code base are considered stable and reliable APIs, and which parts module and theme developers should not rely on.

Reviewing for the correct Drupal API usage, using as parent class a class that is not a base class is not correctly using the Drupal API.

I also asked a question in my previous comment, but that has not been answered from the applicant.

Since the purpose of that section does not seem clear, let me ask this: Are there classes implemented by Drupal core or Drupal core modules, which are not usually used as parent class from classes implemented by contributed modules, for which the constructor is not called (not even from Drupal core), and for which none of the properties are used by a contributed module?

More importantly, IIRC this queue has approved applications extending non API classes in the past year.

The only time I recall I did was because Drupal core did not define any base class that should be extended. That is not the case here.

@avpaderno, please note that in my code I have removed all extensions of core classes. I have completely accepted your position. Most of what you said in comments #22 and #23 was in response to points raised by @cmlara in comment #19. I am happy for you and @cmlara to have this discussion, but at the same time I would also like to know what I need to do in order to complete this application.

I also asked a question in my previous comment, but that has not been answered from the applicant.

If I understand correctly, then answering this question is the final thing blocking my application, is that right? From what I can tell, that appears to be the reason you changed the status back to 'Needs work'.

To be honest, I thought this was a rhetorical question, which is why I didn't answer it. Also, the English syntax of the question is fairly complex, so I'm not sure I completely understand it. The only class that immediately comes to my mind that I think satisfies all the criteria in the question is the \Drupal class. Here is my reasoning based on a breakdown of the question:

• "implemented by Drupal core or Drupal core modules" - Yes, the \Drupal class is implemented by Drupal core.
• "not usually used as parent class from classes implemented by contributed modules" - The \Drupal class is not usually used as a parent class by contributed modules.
• "for which the constructor is not called (not even from Drupal core)" - I don't believe the \Drupal class is ever instantiated, which means its constructor is never called.
• "for which none of the properties are used by a contributed module" - The properties of the \Drupal class are not used by contributed modules, although its static methods can be used by contributed modules.

Perhaps there are other classes that meet these criteria, but I am not able to think of them at the moment. If this is not the answer you were looking for, please re-ask the question in a different way so that I can understand it better, and I will give another attempt at answering.

The \Drupal class, like the \Drupal\Component\Utility\Byte, \Drupal\Component\Utility\Color, \Drupal\Component\Utility\Crypt, \Drupal\Component\Utility\DiffArray, \Drupal\Component\Utility\Environment, and the \Drupal\Component\Utility\Html classes, just to list some of them, defines only static methods. It means there is a set of classes for which the following text applies and applies only to them.

In general, only interfaces can be relied on as the public API.
With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do.

Those classes do not implement interfaces, and they are not base classes. Basing on the rest of Drupal core backend backwards compatibility and internal API policy (Drupal 8 and above), those classes should not be used. The quoted text is saying Maintainers can use them, but be prepared to update their code because of changes in those classes.

As a side note, Drupal 12 will rename \Drupal\Core\Render\Element\FormElement to \Drupal\Core\Render\Element\FormElementBase making clear it is a base class.
(See also RenderElement and FormElement base plugin classes are deprecated and renamed to RenderElementBase and FormElementBase → .)

Automatically closed - issue fixed for 2 weeks with no activity.