-
-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve file sorting performance #6112
Improve file sorting performance #6112
Conversation
c308e4a
to
5e72c55
Compare
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. |
5e72c55
to
d72899a
Compare
d72899a
to
aa54276
Compare
@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)? |
@distantnative you are right. Makes more sense! |
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 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. |
@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 |
7102605
to
5fe843d
Compare
@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) |
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. |
@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. |
Sure, I wasn't saying that one approach is right or not. Just saying that one might have more future consequences than the other. 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 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: |
But in your example, |
@bastianallgeier I am fine with first merging this, but we should add an issue for the lack of checking field sets |
d829177
to
ed0bfb6
Compare
It felt better to solve this directly instead of moving it to yet another issue. |
Looks good to me. Tests also only seem to fail because Codecov upload has a hiccup once again |
This PR …
HasFiles::acceptedFileTemplates()
Best to review each commit separately as each contains one contained improvement
Enhancements
Ready?
For review team
Add to website docs release checklist (if needed)