Split File hooks into separate classes

Adding tags to issue description.

There are existing tests for these hooks. We are just moving the code.

Re-ran 3 tests manually and they passed. All tests green.

#6 @kim.pepper Yes this seems like one rare occasion when a core issue clearly does not need tests. It makes sense for feature requests and bugs, though. This is neither.

@kim.pepper Do you think this requires a change request? This being a pure refactor? e.g. module developers will be unaffected since they will continue to use the existing documented hooks without being concerned with the code is behind them?

My understanding is no-one is using these classes directly nor sub-classing them so we should be ok. But happy to hear others views.

Thanks for taking this cleanup! I've related the meta for teaching please comment there on any thoughts on organization.

For a general review can you please remove final from the hook classes at least until 📌 [policy, no patch] Hook classes should not be marked final Active is resolved.

They are already excluded from bc no need to force final.

Does the logger not get autowired without that attribute?

Also to respond to the questions in general these types of issues won't need crs or new tests unless a big is uncovered.

The most likely thing to be found is hidden dependencies.

My only other comment is that @mstrelan has been leading a huge effort to add return types to all hooks.

I'm not sure if it's more or less disruptiv5 to that effort to add return types to all hooks in this module since you're doing cleanup. I'll send a message on slack.

After reading @nicxvan's comments and code feedback I removed the final keyword for affected classes. This seems uncontroversial and so safe to do: it will not affect functionality. Also it has been agreed in policy documents.

Well the point is there is no policy right now so the status quo should remain until it is settled.

Okay.

After acting on code feedback still getting PHPSTAN error so pipeline not green.

Not sure if the PHPSTAN baseline config files deletions in the MR should be reverted.

You can see why phpstan is failing in the pipeline.

Line   core/modules/file/src/Hook/FileHooks.php                               
 ------ ----------------------------------------------------------------------- 
         Ignored error pattern #^Method                                         
         Drupal\\file\\Hook\\FileHooks\:\:filePredelete\(\) has no return type  
         specified\.$# in path /build/core/modules/file/src/Hook/FileHooks.php  
         was not matched in reported errors.

That means there is a rule to ignore a missing return type.

That method had a return type added, that means someone needs to find the rule in the baseline and delete the rule.

Ah, thanks, that makes sense.

After removing ignore rule from phpstan baseline, pipeline is running all green.

Ok I took a pretty through look, I think this is good.

I left two comments for follow ups, but I think they are non-blocking.

Thank you for the code comments @nicxvan. I have responded. I want to be sure that I understand your suggestions.

We have 📌 [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service Postponed already.

I created 📌 Move file_get_content_headers() to a static method on a utility class Active as a follow-up.

Thanks! I didn't see those.

Since you went through updating this module I'd love your insights here 📌 [meta] Clean up hook classes in core Active any thoughts on ways to group them etc or other thoughts or observations would be great.

@nicxvan Yes, I am reading that with interest.

Just one minor question about a empty hook that's left behind. Fine to self RTBC after linking/creating a followup

Added the link to the follow up, so back to RTBC

larowlan → closed merge request !10551

Committed to 11.x - thanks!

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