PATCH: ensure that composite and nested form fields can be included #145
Conversation
|
Looks (at a quick glance) like this might fix #138 |
|
This needs linting fixes applied |
|
@GuySartorelli @dhensby We need to make patching as easy as possible. I am looking at this: I might be reading this incorrectly, but is there a way that these fixes are applied automatically? We need to speed up our development and this sort of thing would just make everything easier and faster. I am particular looking at this: |
|
Yep, they can be fixed automatically: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Fixing-Errors-Automatically#using-the-php-code-beautifier-and-fixer |
|
@dhensby - thank you for your super quick replies. what i mean is that any commits automatically get cleaned up for stuff like that. I looked if something like this exists - I could not find something obvious. I think GitHub Actions could solve this though. This would save oodles of time. |
|
@sunnysideup a pre-commit hook could do that - I believe you can use pre-commit hooks locally without affecting other developers' workflow. |
|
@GuySartorelli - thank you for writing back. The whole point is that it should not be a local thing, but rather a standardised server thing. If we want to make contributing as easy as possible then this would be a big time saver. In other words, a way to save time for both committer and reviewer is to lint the code, centrally, when you make a commit. i.e. in terms of the style errors that can be fixed automatically -
|
|
There are arguments that could be made both ways. It's also a good opportunity for developers to be mindful of the quality and style of their code.... In any case this discussion is probably better suited to the forums. It's definitely worth discussing and might be a useful change to make. The forums are where it has the best chance of getting attention from enough core committers to be implemented. |
|
In my view it's pretty typical that a project expects contributors to be able to make sure their contributions adhere to the coding styles of the project. This is pretty simple and it's usually possible to automatically fix problems using the tooling provided (as in this case). Sometimes the linting can't be fixed automatically and in those cases the author will need to manually rectify the problems. The problem with automating this process and having CI or a linting bot do the fixes is that we then end up with commits that are solely for linting (which isn't great, but not a blocker) but what does happen is someone else starts contributing commits to the PR. That requires the author to provide the bot or maintainers with push rights to the branch and it also means the history in the PR diverges from the author's version. Diverging PRs become a problem when authors don't realise that commits have been added and so start to get conflicts or issues when they try to update the PR. It is my experience that a typical contributor will not be aware of how to resolve these problems and probably won't understand why they've happened. I'd suggest that a contributor that doesn't know how to lint their code probably doesn't know how to deal with divergent branches in git either. Automating linting for contributors is pretty straight forward too. IDEs often prompt visually to linting errors, provide automated fixes, and can be configured to lint-on-save. I do think it falls into the role of contributors to lint their code before supplying it and, as others have said, there are ways for people to automate that in their workflows. On balance, I think any attempt at automating linting will cause greater pain over time than getting contributors to adopt good habits and to lint their contributions. |
|
Thank you for your considerate and in-depth answers. Here are a couple of things to consider (if you have not already):
If you can see I have a point then I will post on the forum. If I am just trying to justify my own laziness then I am happy to shut up also ;-) |
|
I definitely thinks it's worth raising in the forum's; I've seen this pop up a few times in other PRs as well so you're not alone in wanting this. At the very least it would give a central place for this discussion. |
|
@dhensby discussion above notwithstanding, looks like the requested changes were made. |
There was a problem hiding this comment.
Does not seem to work
Testing locally, the query string for a reqular checkboxe changed from ?filters[Chx]=1 to ?filters[filters[Chx]]=1 and thus stopped working
I tested this using the following code
<?php
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\CheckboxField;
use SilverStripe\Forms\CompositeField;
use SilverStripe\Forms\FieldList;
use SilverStripe\Reports\Report;
class MyReport extends Report
{
public function title()
{
return 'My report';
}
public function sourceRecords($params = null)
{
$filters = [];
if (isset($params['Chx']) || isset($params['Chx2'])) {
$filters['Title'] = 'Contact Us';
}
if (isset($params['Chx3'])) {
$filters['ID:GreaterThan'] = 5;
}
return SiteTree::get()->filter($filters);
}
public function columns()
{
return [
"ID" => [
"id" => "ID"
],
"Title" => [
"title" => "Title"
],
];
}
public function parameterFields()
{
return new FieldList(
new CheckboxField('Chx', 'Filter Title by "Contact Us"'),
new CompositeField([
new CheckboxField('Chx2', 'Filter Title by "Contact Us"'),
new CheckboxField('Chx3', 'Filter by ID > 5'),
])
);
}
}
|
@emteknetnz - thank you for testing. I wonder if that is something that is
|
|
I believe this is related to #169 and should be sorted in latest release. |
Issue #138