Improve file sorting performance

[distantnative]

This PR …

• Add unit tests for HasFiles::acceptedFileTemplates()
• Restore API endpoint, but create own request Panel in site area to use without return

Best to review each commit separately as each contains one contained improvement

Enhancements

• Significantly improved file sorting performance (thanks @rasteiner)

Ready?

• Unit tests for fixed bug/feature
• In-code documentation (wherever needed)
• Tests and checks all pass

For review team

• Add changes to release notes draft in Notion
• Add to website docs release checklist (if needed)

[bastianallgeier]

Maybe we could kill two birds with one stone here. We want the Panel to be independent from the API anyway. We could keep the return value for the API call to avoid breaking changes. But have our own sorting request in the site area.

[distantnative]

@bastianallgeier sorting request in the site area or would it make more sense to add API route support to sections (like we have for fields)?

[bastianallgeier]

@distantnative you are right. Makes more sense!

[bastianallgeier]

Looking at this again, I was wondering if we should move the acceptedFileTemplates method to the Blueprint classes instead.

[rasteiner]

Looking at this again, I was wondering if we should move the acceptedFileTemplates method to the Blueprint classes instead.

Currently there's a one to one mapping between model instances and their blueprint (you create a new blueprint for every model instance, even if it's the "same" blueprint as that of other models). Therefore every blueprint instance actually refers to a single instance of a model. The inherent idea of "blueprints" would however suggest that the same blueprint would be used by multiple models.

As long as there is this 1 to 1 mapping, I don't think it really makes a difference (other than semantics): it's the same code with the same outcome, simply executed in another place.
If you intend to break this mapping in the future (for example to improve efficiency) and have the same blueprint instance for many models, placing the function in the blueprint rather than the model would make a difference if, for example, the accepted file templates depend on a specific model (e.g. a model accepting only a certain number of a file type).

[distantnative]

@rasteiner but I think @bastianallgeier is right, cause even if blueprint object were unique per blueprint (and not 1:1 for each model), the logic in acceptedFileTemplates doesn't depend on the model but the blueprint.

[distantnative]

@bastianallgeier I think I could use help with creating the API endpoint for the section (aka also first adding API endpoints feature for sections as we have for fields)

[bastianallgeier]

Ok, for some reason I totally thought that we already had the section API, which is obvisouly not true 🙈 I think it's a good idea to add it to 4.2 as well and then get this part of the task done. It's not super complicated. But it's definitely beyond the scope of this PR. Let's wrap this one up first by adding the unit tests and reverting the breaking change in the API route. I will start a new branch with the section API. I've already started working on it.

[bastianallgeier]

@distantnative I've added unit tests and refactored the detection for templates in fields a bit more. I also realised by doing so that we completely ignore fields in fieldsets so far. I would suggest to handle this in a separate PR though. It's going to be quite a challenge.

[rasteiner]

@rasteiner but I think @bastianallgeier is right, cause even if blueprint object were unique per blueprint (and not 1:1 for each model), the logic in acceptedFileTemplates doesn't depend on the model but the blueprint.

Sure, I wasn't saying that one approach is right or not. Just saying that one might have more future consequences than the other.
Technically (even though this isn't implemented yet), having a page blueprint like:

sections:
  files:
    template: foo
    max: 1

could mean that some models would accept a new "foo" file, while others (which already have a "foo") might not.

I guess it's therefore debatable if "acceptedFileTemplates" is a property of the model or the blueprint: the blueprint specifies the same rules for many models, but those same rules might give different results depending on the model. So, finally, is it the blueprint which accepts a file template, or is it the model? (honest question)

Should you decide to decouple model and blueprint, having this functionality in the blueprint, when results depend on the model like in the example above, would bring back the need for a static WeakMap<HasFiles, array> to cache results in the blueprint. While having it in the model instead, the cache would stay a "simple" array.

In the end, however, I guess it doesn't really matter and this is bikeshedding. If it were up to me to design this from ground up, I'd probably go for a façade function in the model which caches the results of a call to a blueprint function which accepts a model as parameter. (pseudocode: return $this->cache = $this->blueprint()->acceptedFileTemplates($this);).

[distantnative]

But in your example, foo would always be an accepted file template from the model/blueprint. The specific section is defining a limit which shouldn't limit what this method returns. I know that this might open up possibilities where changing a file template somewhere ends up in this section having 2 entries. This however is really this area where we hit limits with the section based rules and we have to accept some shortcomings. We've seen it with other occasions, such things would only be solved properly with page-wide definitions, not sections.

[distantnative]

@bastianallgeier I am fine with first merging this, but we should add an issue for the lack of checking field sets

[bastianallgeier]

It felt better to solve this directly instead of moving it to yet another issue.

[distantnative]

Looks good to me. Tests also only seem to fail because Codecov upload has a hiccup once again