🇺🇸United States @brandonpost

@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.

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.

@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.

@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.

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.

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.

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

brandonpost → created an issue.

Here's another patch file that includes the change I made, and also the change @praveen-rani made in the MR. It seems like a good idea to implement both changes. I'll commit and post a new release. Thanks to both of you for your help!

Hello @Amandeep123, here's a patch that I believe will fix this issue. Please let me know if it doesn't fix it.

Excellent, thanks for testing, and glad it worked! I'll commit and post a new release.

Hi @praveen-rani, thanks for the quick feedback on this new module! Here is a patch that I believe will fix this issue. Please let me know if it doesn't work for you.

Great! That idea is implemented in patch #107 if you'd like to review it, although the tests failed on the patch because of those other separate issue with the tests.

I'm not sure if it violates any standards/policies to use the setSetting()/getSetting() functionality on BaseFieldDefinition in this way. If so, we could look for another method of setting the flag.

Looking at the remaining tasks in the issue summary, it looks like one of the main things left to do is address the issue mentioned in #63, #66, and #77. The way this was worked around in #93 was to bypass the result of hasColumnChanges() using this code:

if ($this->hasColumnChanges($storage_definition, $original)) {
  $type = $storage_definition->getType();
  if (!in_array($type, ['timestamp', 'changed', 'created'])) {
    throw new FieldStorageDefinitionUpdateForbiddenException('The SQL storage cannot change the schema for an existing field (' . $storage_definition->getName() . ' in ' . $storage_definition->getTargetEntityTypeId() . ' entity) with data.');
  }
}

The problem here is that 'timestamp', 'changed', and 'created' fields will forevermore be able to bypass hasColumnChanges().

hasColumnChanges() was added in issues #2544954 → and #2542748 → because SqlContentEntityStorageSchema cannot handle column changes. But in this case, the column changes in the database have already been handled in _system_update_process_timestamp_field() before getting to SqlContentEntityStorageSchema.

So what if we just set a flag on $storage_definition in _system_update_process_timestamp_field() to let SqlContentEntityStorageSchema know that the column changes have already been handled, like this:

foreach ($table_mapping->getAllFieldTableNames($field_name) as $table) {
  $schema->changeField($table, $column_name, $column_name, $specification);
}

// Set flag to let EntityDefinitionUpdateManager know the column changes
// have been handled.
$storage_definition->setSetting('column_changes_handled', TRUE);

// Update the tracked entity table schema, setting the size to 'big'.
/** @var \Drupal\Core\Entity\EntityDefinitionUpdateManager $mgr */
\Drupal::service('entity.definition_update_manager')->updateFieldStorageDefinition($storage_definition);

Then check for the flag in SqlContentEntityStorageSchema before hasColumnChanges() like this:

if (empty($storage_definition->getSetting('column_changes_handled')) && $this->hasColumnChanges($storage_definition, $original)) {
  throw new FieldStorageDefinitionUpdateForbiddenException('The SQL storage cannot change the schema for an existing field (' . $storage_definition->getName() . ' in ' . $storage_definition->getTargetEntityTypeId() . ' entity) with data.');
}

It seems like this issue of synchronizing changes in database schema with the field storage definition stored in config would also affect how custom modules update the schema for their custom entities. @catch mentions this in the last paragraph of 2542748-24 → . How is it that custom modules work around this hasColumnChanges() issue to change the columns of a field on a custom entity? Couldn't the same thing be done here to update these timestamp fields? (Sorry, I'm a longtime D7 developer, so I'm still getting up to speed on all things D10/11, like how things like custom entity updates work.)

About the failed tests, I took a look at them, but don't see why they would fail. The one test says it fails to find the success message in dblog. But that must mean the other tests that check for the new field specifications passed. It seems hard to believe that the code would succeed in setting the new field specs, but fail in setting a simple log message. I would be interested to see the html it fetched from $this->drupalGet("admin/reports/dblog/event/$event") to see if it actually got a dblog page or maybe a 404 page or something else.

The other test that fails just says 'Unknown', so I don't know what to make of that or what test it is even referring to.

Sorry @catch, I didn't do my due diligence before posting the last 2 patches. I just grabbed the last patch posted and rerolled it for 10.1.x and 11.x. I grabbed patch #98, which was a reroll of #89, which means it missed the work you and @quietone did in #91 - #95.

Here is a reroll of #93 for 11.x

Here's a re-roll for 11.x

Actually, here's a re-roll for 10.1.1. #98 didn't apply completely clean on 10.1.1 because of a couple offsets.

#98 worked for me on 10.1.1, PHP 8.2, and MySQL 8. Thank you!