Pull Request: Refactor FastCGI Client into Separate Package (#4378)

[eanavitarte]

Hi Caddy team,

Looking at the list of issues about FastCGI, I noticed a "help wanted SOS" issue #4378 requesting the FastCGI client be moved into its own package (pretty logic). And as I was already familiar with this codebase (because when I contributed in the v2.7.3 release, I read the whole package), I've prepared a pull request to address this issue. I hope to help you!

The Problem

Currently, the FastCGI client code is intertwined with Caddy's FastCGI implementation, making it harder to maintain (and read!) and potentially reuse the client elsewhere. This refactor aims to improve the code's organization and clarity.

The Solution

1. Moved client files: The files writer.go, record.go, reader.go, pool.go, and header.go are now in the fastcgiclient subpackage. They were non exportable auxiliar FastCGI client files.
2. Created fastcgi.go: It was created a fastcgi.go file in the subpackage just for clean handling the initialization of the noopLogger, previously a single line of the fastcgi.go in the previous implementation. (This is a noopLogger, so it can be safely initialized inside the client, because it just discard the output, but we can now easily refactor the logging in a future)
3. Exported Client: The Client type and its fields are now exportable, so they can be imported in the Caddy's FastCGI implementation package, without changing their behavior.
4. Updated imports: The main FastCGI package now just imports the client, simplifying its dependencies.
5. Maintained compatibility: The commented-out ´client_test.go´ file remains for reference, but it is continue been disabled due to performance concerns, as it was.

Testing

The majority of changes are fast revisable code edits and movements, easily verifiable using Git (they're mainly rename trackings and single lines). It do not change any code logic (I was careful about that).

To thoroughly test the changes, I successfully ran a WordPress site using a Caddy server with this modification. All functionality, including file uploads, API access, and GET/POST requests, worked seamlessly.

Closing Remarks

Well, I hope this solve the 3 years issue #4378 and makes the FastCGI codebase cleaner and more maintainable.

I'd hope to be included in the contributor list for the next release :)

Best regards,

[mholt]

Did a quick first pass. Thank you for working on this! With a few iterations I think we can merge something along these lines into the mainline.

[mholt]

Sorry for my long delay on this.

The changes are looking good!

I have another thought now that I see this with fresh eyes -- let me know what you think.

[mholt]

Being an exported function, this should probably return an exported type. Otherwise getting documentation on how to use it will be irritating.

If we don't want to export the client type, maybe we can return an interface type instead, but honestly, given what NewClient() is doing, I feel like we could just export this as the Client type and I dunno if we even need NewClient() anymore.