feat: sitekit resource loader

[sitepark-veltrup]

No description provided.

[codecov]

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

[sitepark-schaeper]

I think we should follow the common practice to call Exception classes "Exception" - InvalidResourceException and ResourceNotFoundException in this case. I'd also change the namespace to the singular Exception instead of Exceptions.

The Resource class may be better of beeing abstract or even an interface. Either way we should try to avoid constructors with this many arguments.

Lastly, is there a specific reason why all tests are missing declare(strict_types=1)?

[sitepark-veltrup]

I think we should follow the common practice to call Exception classes "Exception" -

It is now very common to omit the exception suffix. This information is also redundant, since it is contained in the package name.

See e.g.

• https://kevlinhenney.medium.com/exceptional-naming-6e3c8f5bffac
• https://phpconference.com/blog/structuring-php-exceptions/
• https://developer20.com/how-to-name-exceptions-its-not-so-obvious/

I would like to stay with the form without extension suffix because I find the information redundant and so everything is just easier.

[sitepark-schaeper]

I would like to stay with the form without extension suffix because I find the information redundant and so everything is just easier.

Generally I'd agree that suffixes depicting superclasses or namespaces are redundent.
In this case ResourceNotFound is probably unmistakably easy to understand. But InvalidResource on the other hand could also be for example a subclass of a ResourceLoadingResult. To follow through with the advice from those articles one would have to name it ResourceIsInvalid instead.

Either way, somehow this feels off. I can't really tell wether this is just my perception or if Exceptions may really be an "exception". Could we gather a couple more opinons on this matter?

[sitepark-veltrup]

Either way, somehow this feels off. I can't really tell wether this is just my perception or if Exceptions may really be an "exception". Could we gather a couple more opinons on this matter?

In agreement from the last meeting, I add the Exception suffix.

[sitepark-schaeper]

These two points from my last comment might have gotten lost:

The Resource class may be better of beeing abstract or even an interface. Either way we should try to avoid constructors with this many arguments.

Lastly, is there a specific reason why all tests are missing declare(strict_types=1)?

I have nothing else to add other than that 👍

[sitepark-veltrup]

The Resource class may be better of beeing abstract or even an interface. Either way we should try to avoid constructors with this many arguments.

I don't know exactly what we're doing with this class yet. So I would leave that open for the time being.

Lastly, is there a specific reason why all tests are missing declare(strict_types=1)?

No, that was not intentional. It was added.