ryancramerdesign / ProcessWire

Our repository has moved to https://github.com/processwire – please head there for the latest version.
https://processwire.com
Other
727 stars 198 forks source link

PageListSelectMultiple showIf issue in module config #1457

Open teppokoivula opened 9 years ago

teppokoivula commented 9 years ago

PageListSelectMultiple seems to have some issues with showIf. This happens in module config screen, using a separate config class extending ModuleConfig:

class MyModuleConfig extends ModuleConfig {
    public function __construct() {
        $this->add(array(
            array(
                'name' => 'apply_to',
                'type' => 'radios',
                'label' => 'Apply to',
                'options' => array(
                    'non_admin' => 'All non-admin pages',
                    'all' => 'All pages including admin',
                    'page_list' => 'Pages selected from a page list',
                    'selector' => 'Pages matching a selector string',
                ),
                'value' => 'non_admin',
            ),
            array(
                'name' => 'apply_to_page_list',
                'type' => 'PageListSelectMultiple',
                'label' => 'Select pages',
                'showIf' => 'apply_to=page_list',
                'description' => 'Select applicable pages',
                'value' => '',
            ),
    ));
    }
}

Field 'apply_to_page_list' shows up when I select 'page_list' for 'apply_to', but once I make a selection in the page list, the whole field disappears again. I'm assuming that this is related to PageListSelectMultiple, since I'm not seeing the same issue with PageListSelect (or any other inputfield I've tested so far).

Please let me know if there's something I'm doing wrong here, or if you need any additional info. I can set up a test case too if that's helpful. This issue occurs with ProcessWire 2.6.20, tested with OS X + Chrome (not seeing any errors in dev tools), applies to both native admin themes.

ryancramerdesign commented 9 years ago

Teppo, this is an id attribute collision, one I've not seen before. For radio and checkbox fields, PW automatically assigns them an id attribute equal to the field name plus the field value. So the apply_to radios with "page_list" value has an auto-assigned id attribute of Inputfield_apply_to_page_list, which is identical to the auto-assigned id attribute of your apply_to_page_list field. I'll see if there's some way PW might be able to detect the condition and avoid it somehow. But for now, you can fix is just by specifically assigning an id attribute to your "apply_to_page_list" field, like this (see "ADD THIS LINE" below):

class MyModuleConfig extends ModuleConfig {
  public function __construct() {
    $this->add(array(
      array(
        'name' => 'apply_to',
        'type' => 'radios',
        'label' => 'Apply to',
        'options' => array(
          'non_admin' => 'All non-admin pages',
          'all' => 'All pages including admin',
          'page_list' => 'Pages selected from a page list',
          'selector' => 'Pages matching a selector string',
        ),
        'value' => 'non_admin',
      ),
      array(
        'name' => 'apply_to_page_list',
        'id' => 'apply_to_page_list', // ADD THIS LINE
        'type' => 'PageListSelectMultiple',
        'label' => 'Select pages',
        'showIf' => 'apply_to=page_list',
        'description' => 'Select applicable pages',
        'value' => '',
      ),
  ));
  }
}
teppokoivula commented 9 years ago

Thanks, makes sense. I'll implement this workaround for the time being. Sounds like it shouldn't use "field name format" for these, though; wouldn't it be safer to use a separator that can't be reproduced using field names, something like a colon or a dot?

Another, somewhat less sophisticated solution would be a (JS?) check for duplicate IDs in such situation, and displaying an error (and perhaps disabling showIf rules if applicable).

ryancramerdesign commented 9 years ago

I have just now looked into the PHP side to see if detection can be done there, but since the individual Inputfield classes don't know the details of the parent form, and the parent form doesn't know the implementation details of individual Inputfield classes, it doesn't seem possible to detect it there. It could feasibly be done with a before form render hook assigned by the radios/checkboxes field, but that ends up being a lot of code and some overhead for a very rare situation, so doesn't seem like a good route.

wouldn't it be safer to use a separator that can't be reproduced using field names, something like a colon or a dot?

Yes it would be and I think that's what we'll have to do. But I don't know what other code might be relying on the current implementation, so I'm going to set this one for "revisit later" since the risks of changing it before a new PW release are greater than the risks of leaving it there (as this is the only time I've ever seen that type of id collision occur).

Another, somewhat less sophisticated solution would be a (JS?) check for duplicate IDs in such situation, and displaying an error (and perhaps disabling showIf rules if applicable).

This would definitely be safer, but since an id attribute is only supposed to exist once in a document, I'm not sure how reliable it would be for JS to detect duplicates, or if it even can. I'll try and find out.

teppokoivula commented 9 years ago

Revisiting this later makes sense. Perhaps this one should be pushed forward until to 3.0, where some breaking changes are more or less expected and/or unavoidable :)

There's a workaround for duplicate ID checks mentioned here: http://stackoverflow.com/a/509965. You can't find items, but you can find attributes and compare them yourself. Downside is that this too adds some extra overhead.