Problem/Motivation

system_settings_form() has traditionally served two purposes:

  1. It provides convenient no-thinking-required variable storage for module developers.

    The config system no longer allows no-thinking-required variable storage; developers are required to ship an xml file with their module that specifies how they want their data stored. This is great for fixing the deployment problem, but somewhat inconvenient for module developers, who can no longer define their storage mechanism just by writing a form.

  2. It keeps us from having to write a bunch of redundant submit handlers.

    system_performance_settings_submit() currently looks like this:

    function system_performance_settings_submit($form, &$form_state) {
      $config = config('system.performance');
      $config->set('cache', $form_state['values']['cache']);
      $config->set('cache_lifetime', $form_state['values']['cache_lifetime']);
      $config->set('page_cache_maximum_age', $form_state['values']['page_cache_maximum_age']);
      $config->set('page_compression', $form_state['values']['page_compression']);
      $config->set('preprocess_css', $form_state['values']['preprocess_css']);
      $config->set('preprocess_js', $form_state['values']['preprocess_js']);
      $config->save();
    }
    

    This is a lot of copy/paste that it would be nice not to have to do.

Proposed resolution

The original CMI commit pulled in a version of system_settings_form_submit() that relies on form_state to store the name of the config object to save the settings to. heyrocker says this was accidental and would like it reverted (see #1496638: Roll back system_settings_form_submit()).

Approaches suggested so far include the following:

  1. Use $form_state['config'] to store config information in $form_state. Then system_settings_form_submit() can load up the config object specified in $form_state['config'] and save $form_state['values'] straight into that. This is the approach that got committed accidentally.

  2. Use form_id to determine config information, such as $form['system.performance:caching.cache']. Then system_settings_form_submit() would check the form_id and save to the specified element (caching => cache) in the specified config object (system.performance).

  3. Extend FAPI to take config objects and turn them into form elements that know where their data should be saved. (See #24.)

  4. Pass config objects to validation/submit handlers, along with $form and $form_state. Form builders would have to write a submit function that migrates $form_values into the config object, perhaps using a simple helper such as https://gist.github.com/2172645. (See #20.) Then system_settings_form_submit() could either save the config object, or be removed entirely in favor of having the individual submit functions save their own config.

  5. Dump system_settings_form() entirely and let module developers write their own submit handlers.

Remaining tasks

Come to a consensus and write a patch.

User interface changes

None.

API changes

The code at #35 proposes the following change to the use of system_settings_form:

Before (D7)

/**
 * Implements hook_menu().
 */
function mymodule_menu() {
  return array(
    'admin/config/mymodule' => array(
      'title' => 'Configure My Module',
      'description' => 'Make my module work how you want it',
      'page callback' => 'drupal_get_form',
      'access arguments' => array('administer my module'),
      'page arguments' => array('mymodule_settings_form'),
  );
}

/**
 * Form builder for admin settings form.
 */
function mymodule_settings_form($form, &$form_state) {
  $form['mymodule_foo'] = array(
    '#type' => 'checkbox',
    '#title' => t('Do you want foo?'),
    '#description' => t('Enable foo for all users on the site.'),
    '#default_value' => variable_get('mymodule_foo', FALSE),
  );
  return system_settings_form($form);
}

After (D8)

/**
 * Implements hook_menu().
 */
function mymodule_menu() {
  return array(
    'admin/config/mymodule' => array(
      'title' => 'Configure My Module',
      'description' => 'Make my module work how you want it',
      'page callback' => 'config_get_form',
      'access arguments' => array('administer my module'),
      'page arguments' => array('mymodule_settings_form'),
  );
}

/**
 * Form builder for admin settings form.
 */
function mymodule_settings_form($form, &$form_state) {
  $form['mymodule_foo'] = array(
    '#type' => 'checkbox',
    '#config' => 'mymodule.settings:foo',
    '#title' => t('Do you want foo?'),
    '#description' => t('Enable foo for all users on the site.'),
    '#default_value' => variable_get('mymodule_foo', FALSE),
  );
  return system_settings_form($form);
}

Original report by beejeebus

Original title: getting from a flat $form_state['values'] key to the corresponding $form element
Originally filed against the forms system

for the CMI initiative, we ported system_performance_settings() to use the new config system.

in the process, we hit problems, as per these code comments i left in 'modules/system/system.admin.inc':

  // TODO: This is a complete hack. The idea is - we need to stash some state
  // with this form, so that when it comes time to save, we can go from a key
  // in $form_state['values'] to that state. I started trying to put this state
  // with each form element, but I have NFI how to get to a form element from
  // a key in $form_state['values'].

the link for more context is:

http://drupalcode.org/sandbox/heyrocker/1145636.git/blob/refs/heads/8.x-...

the hack was to stuff a bunch of stuff in $form_state, so we could avoid having to get from a flat $form_state['values'] array back to arbitrarily nested $form elements.

was i just Doing It Wrong, or do we have a core FAPI issue here?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

I discussed this with effulgentsia at PNWDS at length. We reached the conclusion that the best would be if the form structure would always reflect the data structure while the form is being worked on (retrieve, alter, build, validate, submit) and only in the rendering phase would we move the elements in the place dictated by the DOM tree.

Most likely we would fire a hook from #pre_render of #type form elements. In D8 we can simply patch system_element_info and in D7 a contrib can alter it. Alternatively we can use some preprocess maybe?

Let's see an example. We have $form['revision_information']['revision'] in the node form. We change this to $form['revision'] and in said hook we do <?php $form['revision_information']['revision'] = $form['revision']; unset($form['revision']);.

Edit: to clarify this is just best practice. We do not need to change anything at all in form or render API to make it work. The forms themselves need to change, yes.

webchick’s picture

Ehhh. That approach sounds like it'll make authoring the form as a module developer insanely more difficult.

What about a form_values_smoosh() function or something? :P

Anonymous’s picture

i don't know where to go with this.

i spent a bit of time working on a form object that didn't smoosh 'structure and processing of form' with 'how will it look in html'.

i tried a layer of 'uglifying' the internal structures of the object to produce an array that would work in the render system, but it didn't really feel like much of an improvement.

so, yeah. i'm going to stop complaining and just live with it.

gdd’s picture

I agree with chx that separating the field definitions from the display makes sense, but I think we can make it simpler and not require the display information to be injected in a hook. How much work would it be to just change the way forms are defined so that the two pieces are separate? Something like this

$form['caching'] = array(
  '#type' => 'fieldset',
  '#title' => t('Caching'),
);

$form['cache'] = array(
  '#type' => 'checkbox',
  '#title' => t('Cache pages for anonymous users'),
  '#default_value' => $cache,
  '#weight' => -2,
);

$form['cache_lifetime'] = array(
  '#type' => 'select',
  '#title' => t('Minimum cache lifetime'),
  '#default_value' => variable_get('cache_lifetime', 0),
  '#options' => $period,
  '#description' => t('Cached pages will not be re-created until at least this much time has elapsed.')
);

$form['page_cache_maximum_age'] = array(
  '#type' => 'select',
  '#title' => t('Expiration of cached pages'),
  '#default_value' => variable_get('page_cache_maximum_age', 0),
  '#options' => $period,
  '#description' => t('The maximum time an external cache can use an old version of a page.')
);

$form['#render'] = array(
  $form['caching']['cache'],
  $form['caching']['cache_lifetime'],
  $form['caching']['page_cache_maximum_age'],
)

I'm not really familiar with form api but in theory couldn't something along those lines work without too much trouble?

Anonymous’s picture

i think #4 is well worth looking in to.

effulgentsia’s picture

In general, $form_state['values'] is not flat. It only is when nothing sets #tree, which is usually the case for settings forms, but not for complex forms, like ones with Field API fields, which include necessary depth within $form_state['values'] for field name, language, item number (delta), and component (column).

That $form and $form_state['values'] both have depth, and often have *differing* tree structures is one of the things that introduces complexity and confusion for a lot of developers. Form elements contain a #parents property (once form_builder() is complete) for mapping from the element to its value location in $form_state['values'], but there's currently no facility to go the other way, from $form_state['values'] to the element, which is what this issue is about.

chx and I did discuss that there would be benefits to simplifying things by requiring that $form be structured the same as $form_state['values'] and introduce other patterns for providing render structure to $form, but that's just an idea at this point; it's something that needs to be worked out, which can take a while, so we should not let it impede the CMI initiative.

My first reaction to this issue is that there's something architecturally wrong with needing to go from data in $form_state['values'] to the form element that provided the UI for that data element. I see the use-case here for system_settings_form_submit(), but I wonder if it's because our architecture for system_settings_form() is flawed. Is there any other use-case from CMI where this comes up, or is it just about handling system settings forms?

gdd’s picture

Thanks this information is really helpful.

So far this is just about settings forms, but we also haven't had much of a chance to experiment with other similar but more complicated implementations. The idea here is that we added information about what file each value is stored in to the form element declaration, then after submission we go find that data and save the values there. It seemed natural to just iterate through the values and go get the information related to each one, but it might be possible to do it the other way too. I will try and take more of a look at this.

gdd’s picture

So here is system_settings_form_submit() as it stands now

function system_settings_form_submit($form, &$form_state) {
  // Exclude unnecessary elements.
  form_state_values_clean($form_state);

  foreach ($form_state['values'] as $key => $value) {
    if (is_array($value) && isset($form_state['values']['array_filter'])) {
      $value = array_keys(array_filter($value));
    }
    variable_set($key, $value);
  }

  drupal_set_message(t('The configuration options have been saved.'));
}

So given what effulgentsia said in #6, it seems like we should be able to rewrite this to iterate the $form elements and map them back to $form_state['values'] instead of the other way around right? I'm curious what the implications of this would be, in theory it wouldn't break any existing forms right?

effulgentsia’s picture

Just brainstorming here...

While in general $form_state['values'] can be a tree, system_settings_form_submit() already makes assumptions that the top-level keys are special (at least in D7, where they're the basis for variable_set()). If it makes sense in CMI, to continue treating $form_state['values'] as essentially flat (the values can be arrays, but if only the top-level keys determine which config object manages it), then what if we extend the signature of system_settings_form($form) to system_settings_form($form, &$form_state, $config), where $config can contain the mapping array that you currently have in the sandbox as $form_state['config']. system_settings_form() can then internally set $form_state['system_settings_config'] = $config or maybe if we want to flag this as private, even $form_state['_system_settings_config'] = $config, since only system_settings_form_submit() should need this, at least until we learn otherwise.

My point is that I think the current $form_state['config'] mapping makes sense for now, but only in the context of system settings forms. However, if the use-case turns out to be more general, then we may need to change it to an iteration of $form. However, chx hates adding extra tree traversal in PHP, since recursion in PHP is slow, so let's hold off on it until there's a use-case.

effulgentsia’s picture

Actually, I'm still not clear on why we have this:

$form_state['config']['cache']['path'] = 'caching.cache';
$form['caching']['cache']['#default_value'] = $config->get('caching.cache');

Why not:

$form['caching']['cache']['#default_value'] = $config->get('cache');

In other words, what is 'path' actually needed for?

gdd’s picture

In the new system configuration is stored in XML files. This is the file for the system performance settings:

http://drupalcode.org/sandbox/heyrocker/1145636.git/blob/refs/heads/8.x-...

As you can see, these values may be nested. Additionally, the settings for a single form may span multiple files. In this case, block module form_alters in a value, and as such its initial defaults are kept with the block module

http://drupalcode.org/sandbox/heyrocker/1145636.git/blob/refs/heads/8.x-...

So when we save data from a settings form, we need to know a) what file we're writing to ('name') and where in the xml we're writing to ('path') for each field we're saving. The initial idea was we would add this as a new piece of data to form element definitions (like '#config_path' and '#config_name' or something), and that is when we ran into the current problem of mapping from values back to form elements and made the mapping system instead which is a total hack.

Does this context help any?

effulgentsia’s picture

It makes sense to me that, in general, you want to support $form_state['values']['cache'] going into some XML file at the path /foo/bar/cache. It doesn't make sense to me that the path you want should have any relation to the structure in $form. In other words, if you have some reason for wanting it /caching/cache, then okay, but to me that's an independent decision from the UI for that data being within $form['caching']['cache']. In other words, storage needs depend on the data, and an HTML form needs to interact with that data, but there's no reason why storage location needs to be coupled to the HTML structure of the form.

So, actually, I think $form_state['config'], as you have it, is a fine approach for defining storage information for $form_state['values'], and that putting storage information directly into the $form element would actually be worse. This maintains what I think is the right decoupling: $form elements have a single #parents key that controls the name attribute for HTML, the result of which determines the structure of $form_state['values']. A separate variable, $form_state['config'] provides information on how to store $form_state['values'] that's completely independent of the structure of $form.

msonnabaum’s picture

It may feel wrong architecturally to have storage coupled with what is mostly HTML structure, but that's how system_settings_form() currently works, and I have to admit I find it kind of nice from a DX perspective.

I'd love to see storage locations determined based on form structure unless you're doing something like the block module does here, in which case you have to specifically declare the config tree that data needs to be saved to. Really, I'm not sure why we can't just key it off of form id? In this case it'd look something like config/system-performance-settings/cache_lifetime/0, which seems pretty sensible to me. Perhaps that won't make as much sense on all system forms, but I just don't want to see the experience of authoring these forms get more complex than it already is.

gdd’s picture

Hmmm, that does make sense. However it also seems like it is going to add some complexity to creating these forms. One of the advantages of system_settings_form() is its simplicity. Of course in the past we didn't have to specify where stuff got saved, we just repurposed the element name for this. I was just brainstorming this in IRC with msonnabaum and beejeebus and some things were brought up

- It's possible that system_settings_form() just doesn't make as much sense as it used to in the new storage paradigm. We could just dump it and write manual submit handlers for all these forms. However, this would increase code weight quite a bit. There are 26 instances of system_settings_form() in core alone.

- It was pointed out more than once that $form is already used for a lot of things beyond just how the elements are represented in HTML. For instance we add submit functions there. This break is not as clear as it seems at first glance.

- We tossed around varying ways of repurposing form_id and/or form element names for this purpose. For instance $form['system.performance:caching.cache'] would save to the caching/cache element in the file system.performance.xml. Or maybe the form_id becomes the name of the file that the form will save to (although forms can save to multiple files, which is another problem.)

Ultimately, I think, this is something we need to revisit in CMI - how are we going to approach forms saving to files in general, and is the current approach the best one we can take. This merits a separate discussion I think, so I'm going to open that up elsewhere and mark this postponed for the moment.

gdd’s picture

Status: Active » Postponed
gdd’s picture

Title: getting from a flat $form_state['values'] key to the corresponding $form element » Implement system_settings_form() in CMI
Status: Postponed » Active

I changed my mind and decided to refocus this issue instead. I think we have two questions at hand

- Do we just want to dump system_settings_form() and write manual submit handlers for everything?
PRO: Easily implemented without any changes to form api, removes a magic 'drupalism'.
CON: Increased code weight, somewhat decreased DX and increased complexity of implementing simple forms.

- Do we figure out a way to implement system_settings_form() using CMI
PRO: Retains a somewhat simple way to implement simple forms.
CON: It will by necessity be more complex than before because we have to specify storage, not sure the best way to approach it (although it is probably by using $form_state, which seems to be the least ugly of the solutions so far.)

Personally I am leaning towards just not using system_settings_form() in the absence of a good solution but I'd like to hear other thoughts and approaches.

webchick’s picture

I'm not ready to put a formal +1 vote in, but system_settings_form() tends to be a fairly complex "magic" thing to explain to new Drupal developers, so the consistency argument is not to be discounted. At the same time, I do appreciate the nice DX shortcut it allows once you figure it out.

Tagging to see what the thoughts are of folks working on decreasing the Drupal learning curve.

gdd’s picture

Issue tags: +Configuration system

Also tagging cmi which I forgot to do before

Letharion’s picture

I kinda like #14's $form['system.performance:caching.cache']. It's somewhat verbose, but that also makes it clear what's what. Also, this means that the data, when passed on, comes with the right information, like an address on an envelope. It makes sense to me.
Given that I don't know much about FAPI, I guess that's to some extent relevant for the "complexity problem" mentioned in #17.

EclipseGc’s picture

Discussed this a bit with heyrocker in IRC, and wanted to submit a potential solution here.

Someone else previously mentioned passing configuration objects to forms. That's potentially a really good idea (though it may need to be an array of configuration objects depending). In this way we can pass along configuration objects to the validate and submit handlers, and this ends up being very useful because in validate we have canonical "this is the current value that you care about" data that we can compare against (when relevant) and in submit, we can simply migrate values from the form state into the appropriate configuration object. The magic part from there could just be a submit handler that iterates over all the config objects in the array and saves them for you. To put this into heyrocker's previous pro/con format:

PRO: Should actually provide a better DX experience. Won't require forms to get ugly and represent their schema or anything crazy, and will be consistent across all forms that utilize configuration (which should fundamentally be everyone).
CON: Will require submit handlers for anyone currently using system_settings_form(), but these submit handlers would be super straight forward since they're just migrating values in the form state into the configuration objects that the form is controlling.

Eclipse

xjm’s picture

I agree with #20. In my opinion, the only really confusing thing about system_settings_form() at present is that we really don't document it anywhere. The API doc is incredibly sparse, and there's no reference to it in FAPI docs that I could find. There's a a few references in the handbook (http://drupal.org/node/1111260), but those could be made a bit clearer/more prominent as well. So I think with good documentation linked in the right places, some of the confusion could be alleviated.

Sure, new users will tend to think in terms of the fields they want to put on their forms, not in terms of the implied data structure. But I think it will make it a lot easier for modules to support configuration management, and a convenient Drupalism is better than a complicated one. :)

Anonymous’s picture

yeah, i argued for something like #20 in IRC. more code, but much simpler, discoverable code, and we can create a really dumb helper(s) that this code can use.

gdd’s picture

Note that even if we pass around config objects, I think we will still need the mapping array right? Because we will need to know which form element maps to which value in the config object.

Ultimately if we still have to create submit functions, I'm not sure this would buy us much. If we can't make it as magical as the current system is, I think I'd rather just not do it at all.

xjm’s picture

#23: Well, my thought was that we would add form elements from the config objects. Like, a widget for each setting, kinda like Field API (except better DX, and supporting the whole config object better than Field API supports the entity). Instead of writing mapping arrays or submit handlers for every single config form, we provide a Configuration Form API that does this magically.

Edit: The interface for the CF API could be as simple as a magic naming convention. (Though, note that I am not fond of this-that:foo_bar-baz:various-delimiter as a pattern.) Or it could be some #property. Or something totally different if we want, like OO, sky's the limit. The point is that we can provide an API that automatically handles the necessary mapping/submission handling.

Edit 2: One of the down-the-road steps for the Entity API improvements is to provide $entity->form() or suchlike. You get a base form automatically, and then you alter it how you like. The burden is on the API to provide the data, not on the user. That's what I'm thinking.

joachim’s picture

> system_settings_form() tends to be a fairly complex "magic" thing to explain to new Drupal developers,

I can still remember discovering system_settings_form() as a Drupal newbie, and thinking 'Hey wow! Drupal has really neat things that save me doing boring grunt work! That's so cool!'

So a big -1 to removing this 'magic drupalism'. It's good magic.

If the problem is needing to go from $form_state['values']['parent_with_tree']['element'] back to $form['parent_with_tree']['parent_without_tree']['element'], then couldn't the form building process build up an array with the same structure as $form_state['values'], but where instead of form values we have an array of $form parentage, so:

$form_state['build_lookup']['parent_with_tree']['element'] = array('parent_with_tree', 'parent_without_tree', 'element');

IIRC that's effectively the problem I solved for a one-off in http://drupal.org/project/form_email_confirmation.

Alternatively, why not require form elements that want magic saving with system_settings_form() to set whatever stuff it needs?

gdd’s picture

Component: forms system » configuration system

Moving this to configuration system so I can track it more easily.

sun’s picture

chx’s picture

One wonders here -- the problem as stated start from form_values. But, we can grab the $form itself , use #value where given and just use that together with #parents . form_state['values'] is just convenience, a denormalization if you want.

chx’s picture

Roughly (I moved array_filter to be a top-level property):


function system_settings_form_submit($form, &$form_state) {
  _system_settings_set_vars($form, $form);
  drupal_set_message(t('The configuration options have been saved.'));
}

function _system_settings_set_vars($element, $complete_form) {
  foreach (element_children($element) as $key) {
    _system_settings_set_vars($element[$key], $complete_form);
  }
  if (!empty($element['#input'])) {
    $value = $element['#value'];
    // TODO: form_state_values_clean().
    if (is_array($value) && !empty($complete_form['#array_filter'])) {
      $value = array_keys(array_filter($value));
    }
    variable_set($element['#parents'][0], $value);
  }
}
ksenzee’s picture

Assigned: Unassigned » ksenzee

I'm going to do some thinking and talking about this at the Denver code sprint and see if I can come up with a patch to argue over.

ksenzee’s picture

Issue summary: View changes

Added issue summary.

ksenzee’s picture

Assigned: ksenzee » Unassigned

I was thinking of writing a patch, but it turns out the $form_state['config'] approach got committed already. heyrocker suggested that should be reverted until this issue is resolved, so I created an issue over at #1496638. I also added an issue summary to this issue.

The thing I liked about system_settings_form() in D<8 is how it made data storage easy. (Too easy, really.) You didn't have to think about your data storage format. Now that the config system is forcing us to think about our data storage format, we lose that simplicity. No matter what we do to system_settings_form(), the module developer still has to create an xml file, so we're not going to get the same magic we used to have. That magic is going to have to come from somewhere else. So to me, replacing the easy DX of system_settings_form() also means providing an easy way for developers to create that initial xml file. I filed #1496930: Don't make module developers write XML by hand when defining default configuration to think about the best way to do that.

ksenzee’s picture

Issue summary: View changes

minor edits to issue summary

xjm’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Assigned: Unassigned » sun

A first attempt (actually two if you take git history into account) lives in the config-settings-form-1324618-sun branch.

The problem space turned out to be much more complex than I originally imagined, so I'd love to collaborate in the sandbox and on IRC.

sun’s picture

Status: Active » Needs review
FileSize
13.67 KB

FWIW, current state of affairs.

Git history helps to understand this better.

sun’s picture

Title: Implement system_settings_form() in CMI » Re-implement system_settings_form() for the configuration system
Priority: Normal » Critical
Issue tags: -Increases learning curve
FileSize
15.21 KB

Slept over this. Let me introduce to you:

Configuration settings forms NG

  1. Stop attempting to handle both variables and new configuration in a single API.
  2. Kill the dated and ugly anti-pattern of calling system_settings_form() at the end of form constructors.
  3. Leverage state of the art Form API features instead: Shared base form IDs and wrapper callbacks.

    This gives proper hook_form_alter() and theme hooks for free. And, also paves the way for the discussed vision of leveraging a generic property API (à la fields with meta info, data types, validation, and formatters for individual config settings).

  4. Simpler, cleaner, better.

Since this is a total revamp, the new code lives in the config-settings-art-1324618-sun branch.

Also bumping to critical, since we should resolve this ASAP, before converting more and more module variables to the config system.

webchick’s picture

Tagging for an issue summary. It'd be great to document some before/after code to get a sense of what this is doing.

joachim’s picture

> Kill the dated and ugly anti-pattern of calling system_settings_form() at the end of form constructors.

How is that an anti-pattern? It's nice and simple.

That said, if you can replace it with even simpler, I'm all for it.

Reading the patch quickly, I am guessing that you simply do this in any form:

+  $form['simple'] = array(
+    '#type' => 'checkbox',
+    '#title' => 'Simple',
+    '#config' => 'config_test.settings:simple',
+  );

and the result is that you magically get:

- the config value shown as the form element's default
- the user-entered value saved to config when the form is submitted.

If that is the case, then it's new magic I am all in favour of :)

EDIT: However, there is one negative to this change I can see: harder to learn and harder to find the documentation. system_settings_form(), once you found it, was a function with its own documentation which explained how to use it. FormAPI is not a terribly well documented part of Drupal. This change would need an accompanying new section in the Big Monster FormAPI Document.

gdd’s picture

There are a couple pieces of magic that could make this a little easier

- If a form-level config object is specified ($form['#config'] = 'system.performance') then this is the default file used for all settings on the form. Any field-level-items without a colon are just specifying a key in that file.

- If there is no #config item on a specific field, then just use the field's key as the config key.

For simple forms then, you would just need to specify a form-level #config item, and the rest could take care of itself.

Still going through the code.

gdd’s picture

Hah I see some of this was already started in the code. Great minds etc.

gdd’s picture

Issue summary: View changes

Markup/whitespace.

larowlan’s picture

Issue summary: View changes

Updated issue summary to add API change

larowlan’s picture

Added issue summary (proposed API changes) showing before and after.

sun’s picture

FileSize
16.4 KB

And now, approach no. 3) KISS.

Based on #35, thus sharing some implementation concepts, but much simplified.

I've added extensive docs to config_get_form() this time to clarify the design goals and also simple vs. advanced usage.

Code for this approach/direction lives in the config-settings-kiss-1324618-sun branch.

One detail that was definitely wrong with #35 was the attempt to automate the assignment of #default_value element properties. While that sounds like a nice convenience behavior, it's prone to break, badly, because HTTP POST data !== Form API values !== Configuration object data values. That does not completely invalidate the other code of #35 though, so I'm going to remove that #default_value handling (and addition of Form API #preprocess) from that branch.

larowlan’s picture

Needs to be redone in light of #41

sun’s picture

gdd’s picture

Priority: Critical » Major

This is not a release blocker, and while current code is not as nice as it could be (I'll even go so far as ugly), it works fine. Downgrading to major.

alexpott’s picture

One potential issue with the approach in #41 is that all the config data is replaced when saved. Which means (if I'm not mistaken) that the form has to expose every config setting for the config objects you are administering. This limitation was not present in system_settings_form().

On one hand this is probably a good thing as this should mean that there are less orphaned keys. On the other, it means that the flexibility through the ability to inject config into another config's form might not be as useful as intended.

webchick’s picture

Hm. That's also a problem because we do have several hidden, advanced configuration options that we don't want to expose a UI for, at least in core. See settings.php for a bunch of them.

cosmicdreams’s picture

Pro:

I like the way the function wraps the standard syntax of setting things into the config. It simplifies effort for the developer as it reduces the amount of syntactical push-ups that we'll have to do in order to use the new Config API. Every module that sets settings will need to do this.

While at the same time developers using the Config API outside of the context of using a form to set and get values can still use the config api directly.

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/DrupalConfig.php
@@ -114,6 +114,17 @@ class DrupalConfig {
+  public function setData(array $data) {
+    $this->data = $data;
+    return $this;
+  }

This is already in HEAD, from #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML

+++ b/core/modules/config/config.module
@@ -1 +1,127 @@
+  $form_state['build_info']['base_form_id'] = 'config_settings_form';

This is an unfortunate consequence of drupal_retrieve_form() doing something with 'base_form_id' before 'wrapper_callback' is called. We should fix that, and move this line into config_settings_form_wrapper().

+++ b/core/modules/config/config.module
@@ -1 +1,127 @@
+  // Load, replace, and save the configuration objects.
+  foreach ($form_state['values']['config'] as $name => $data) {
+    config($name)->setData($data)->save();
+  }

I agree with the issue comments above that we should do something more like mergeData() rather than setData().

+++ b/core/modules/config/tests/config_test/config_test.module
@@ -0,0 +1,68 @@
+    '#default_value' => $config_other->get('set.first'),

We currently have #value_callback as part of FAPI, but it's overloaded with both input-to-value mapping and default value setting. We should decouple those and introduce a #default_value_callback (or maybe #default_value_callbacks), so that we can have a central function for this, since we have all the info we need in $element['#parents'].

effulgentsia’s picture

Also, let's change a real form, like system_performance_settings(), to use this approach, as part of this issue.

effulgentsia’s picture

Also, instead of a config_get_form() function, how about using hook_forms()? It would require drupal_retrieve_form() to call that hook and apply the definitions even when the $form_id function exists, but I think that's appropriate.

sun’s picture

@effulgentsia: Note that there is a second/alternative approach in #35 - let's make sure to also review that. :)

+ $form_state['build_info']['base_form_id'] = 'config_settings_form';

This is an unfortunate consequence of drupal_retrieve_form() doing something with 'base_form_id' before 'wrapper_callback' is called. We should fix that, and move this line into config_settings_form_wrapper().

Mmm, actually, this particular code is not a work around of any unfortunate consequence, but instead, a clever (ab)use of base form ID concept we have in Form API already. However, this concept and its related functionality is normally only triggered for actual/true base form constructors; i.e., when you try to build a $form_id, for which no equivalent form constructor function name exists; in which case hook_forms() is called, which supplies the actual callback to execute. Only in that case, a base_form_id is assigned and the corresponding form alter as well as theme hooks are taken into account.

Here, the idea is to not actually use or require the usage of hook_forms() (or any certain function naming pattern), but instead, only leverage the base form functionality, which is baked into Form API already. In other words, I don't really want to change drupal_retrieve_form(), because it does exactly what I want ;)

I agree with the issue comments above that we should do something more like mergeData() rather than setData().

Diving into #1496542: Convert site information to config system made me realize that this is indeed not really possible, so I'm going to change that into individual ->set() calls in a follow-up patch.

However, we will have to find a solution for the actual problem space of retaining orphan/obsolete keys and sub-keys within a configuration object forever. That's not really limited to settings forms, so I've created #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*

We should decouple those and introduce a #default_value_callback

Nope, as mentioned in #41 already: The idea of automatically assigning #default_value properties is utterly bogus. While that sounds like a nice convenience behavior, it's prone to break, badly, because HTTP POST data !== Form API values !== Configuration object data values.

sun’s picture

Status: Needs work » Needs review
FileSize
20.17 KB

Had a great call and discussion with @effulgentsia about this issue.

Let's introduce approach #3: #type 'config'

- Removes the unusual #tree = TRUE on the top-level $form.
- Introduces element #type 'config' as a "container"-alike element that developers are used to, at least in some form.
- Thus, removes at least some of the hard bindings on the $form array structure.
- If there was a element-level #submit, then it would allow for mixing + merging configuration settings forms as you like.

Also (and this really helped, so thanks!), as someone requested, this patch contains a second example, the actual conversion of the system.performance settings form to this 3rd approach. (I'll try to incorporate this into the 1st and 2nd approach as well.)

That said:

Please note that the goal for this issue is not to find a picture perfect solution. The goal is to find a working and basically acceptable approach for handling configuration system settings forms, so the plethora of config conversion patches that are happening in parallel right now do not continue to still use system_settings_form() and thus write variables for stuff that's actually converted into config already.

As already hinted at in #1448330-19: [META] Discuss internationalization of configuration, the currently envisioned and final goal for D8 would rather be to abstract Field API's concept of types, meta data, etc not only to the entity level, but instead, turning it into a completely generic property API, so the configuration system is able to leverage the identical functionality. Consequently, this will mean that we're going to revamp/refactor all configuration forms throughout core onto that concept, once ready.

Thus, the goal here is to provide a temporary transition path only. It should make sense, it should work, and it should provide some sane DX (especially important for newcomers/novices). But it should not and does not attempt to be the "final" solution for D8.


P.S.: Given the above battle plan, in case you're asking yourself, what's so different between "redoing config forms" and "redoing config forms" as stated in #1599554: Tutorial/guidelines for how to convert variables into configuration, the answer is: Forms != Data. The form abstraction layer that exposes the interface to manage the data can happily change. The underlying data should only be changed once. As you know, changes to data require upgrade paths. The data format and structure most likely won't change anymore. So we better migrate the data correctly, and only once.


P.P.S.: Had to rebase all of my config-settings branches against latest 8.x, because cherry-picking the YAML commit only "seemed" to have worked out fine at first - only found out a couple of commits later on that git diff 8.x...HEAD produces 200 KB of forward-and-reversed history. :( Looks like I'm ditching that approach and sticking back to a -base branch per feature branch.

Status: Needs review » Needs work

The last submitted patch, config.settings.52.patch, failed testing.

effulgentsia’s picture

The test failures in #52 are due to system.module having a dependency on config.module. Simply adding the dependency to system.info doesn't fix it however, since drupal_install_system() has custom code for installing system.module.

Additionally, I wanted to share a quick thought here that I haven't had time to develop yet: perhaps instead of a 'config' container element, we can make form constructors call a function for each config element. Something like:

$form['caching']['cache_lifetime'] = config_form_element(...);

I think this would map more closely to our eventual goal of using property/widget api for this. For now, this function could take an $element as one of its arguments and do whatever magic it needs with #tree/#parents and add tracking info to $form/$form_state to be able to iterate all config elements in the submit handler without recursion. This iteration would also allow config()->set() to be called for each config element rather than trying to collate/merge (I'm not sure, but I think #52's collate logic might not do what's desired for checkboxes and other compound elements).

Sorry to be shooting this off without developing the idea more myself. I may play with this a bit more this weekend, but not sure when yet. If you want to run with it in the meantime though, go for it.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.97 KB

This lacks comments and documentation, but here's basically what I mean.

The idea of automatically assigning #default_value properties is utterly bogus. While that sounds like a nice convenience behavior, it's prone to break, badly, because HTTP POST data !== Form API values !== Configuration object data values.

I added #default_value automation into this patch, because I don't really buy this reasoning. If FAPI values != Config values, then we can't be automating their saving either, but that's the whole point of this issue. If we can automate their saving, then we can automate their default values. There may be complex data types that present a challenge to both saving and default value initialization, but we need to find those use-cases if we want to worry about them, and in any case, #1448330: [META] Discuss internationalization of configuration is where we can deal with that.

effulgentsia’s picture

FileSize
6.99 KB

Oops. #55 lacked a return statement from config_form_element(). This fixes that.

effulgentsia’s picture

+++ b/core/includes/config.inc
@@ -86,3 +86,34 @@ function config_get_storage_names_with_prefix($prefix = '') {
+function config_form_element(&$form, &$form_state, $config, $key, $element) {
+  $element['#tree'] = TRUE;

I think this might be unnecessary and better to remove, but I'll wait until further comment on the rest of this before submitting another patch.

effulgentsia’s picture

FileSize
7.01 KB

I couldn't help myself. Here's the fix for #57 and some other minor cleanup.

sun’s picture

I like the function/callback direction. Especially, because that's likely going to be the case either way when there is a generic property API. But also, because this allows us to potentially embed language handling into that function. Both of these are separate topics, but this direction seems supportive and compatible.

However, you didn't base your work on any of the existing branches, but we definitely need the test coverage and config_test module, so the next required step is to "rebase" #58 onto the config-settings-element branch and call it config-settings-callback-1324618-effulgentsia. (This can also be done on behalf of @effulgentsia, if someone wants to beat him to it.)

chx’s picture

Can't we get back to the point where we call a single function at the end and it handles all this?config_form_element(&$form, &$form_state, $config, $key, $element) I do not think this is very good DX... especially this many times. We could go around with putting $form['#config'] = $config; return config_form($form); (as $form_state is not used IMO in the proposed config_form_element) and just iterate the form, pick the last form key as a $key and then apply the config_form_element function to it.

bojanz’s picture

I agree with chx that the latest proposal looks awkward, DX-wise. We shouldn't need to do that.

That said, huge kudos to sun and effulgentsia for pushing this forward.

effulgentsia’s picture

FileSize
4.14 KB
7.62 KB

config_form_element(&$form, &$form_state, $config, $key, $element) I do not think this is very good DX

Why is it not good DX? If it's verbosity, we could shorten by removing the unused $form/$form_state, but I was trying to be consistent with the rest of FAPI and Field API, where those are always passed to form functions.

If it's that a function is called at all, I think this is actually a good thing, especially if as per #52, the goal is to eventually move to something similar to the field system's widget API (once it's abstracted from entity fields to generic properties), at which point, very likely a function call rather than a declarative element will be needed anyway.

But, if we want to avoid an explicit function call for every config element, would something like this patch fly? If a generic hook_form_element_alter() is too crazy, we could limit it to some kind of tagging system (like for hook_query_alter()), but that would raise DX questions for setting tags.

just iterate the form

I'm surprised to see that recommendation from you, chx. Normally you're against custom form recursion outside of the built-in FAPI pipeline. That's why this patch does it with a new alter hook instead.

effulgentsia’s picture

Also, I just re-read #38, and that could be incorporated into #62 to simplify further, which I can do in the next patch if people here otherwise like the direction of #62.

For simple forms then, you would just need to specify a form-level #config item, and the rest could take care of itself.

I think we would still need at a minimum element-level '#config' => TRUE so that we don't automatically create config values for every input element.

effulgentsia’s picture

FileSize
11.64 KB

Here's yet another approach that instead of skirting around the lack of databinding capability in FAPI, adds that capability head on. IMO, this is unquestionably better than #62, and I leave it up to you guys to decide on whether or not it's better than #58. Please decide soon, so we can re-add the test coverage requested in #59, refine the docs, and unblock converting a few dozen forms to CMI.

joachim’s picture

Status: Needs review » Needs work

I'm commenting purely as someone who's written a lot of forms -- I don't feel qualified to say much about the internals of FormAPI.

+++ b/core/modules/system/system.admin.inc
@@ -1658,11 +1658,11 @@ function system_logging_settings() {
-  $config = config('system.performance');
+  $form['#config'] = 'system.performance';
+  $config = config($form['#config']);

The second line looks a bit confusing. You don't seem to be doing anything with $config later on.

Didn't we also say further up that forms might want to mix and match config from several buckets? How would that work? Could you set #config as just 'system'?

+++ b/core/modules/system/system.admin.inc
@@ -1683,7 +1683,7 @@ function system_performance_settings($form, &$form_state) {
+    '#databind' => 'config',

I like the concept here -- telling the form element in one go what it connects to, so it knows how to both get its default value and save itself. That's been missing from FormAPI for a long time.

I'm a bit put off by the 'databind' word itself. But that's bikeshed.

What else would it handle? Could it save entity forms from the tedious '#default_value' => isset($entity->foo) ? etc etc?

-4 days to next Drupal core point release.

sun’s picture

Status: Needs work » Needs review

Hrm. #64 makes it "simple" for the original form constructor to handle its own config, but imposes the same complexity of defining $element['#config'] = 'mymodule.settings:parent.childkey' properties for hook_form_alter() implementations on every element as in #35.

In general, the idea of data bindings is not bad though. That said, if we continue with this direction, then I doubt we'll be able to unblock the config conversion patches anytime soon, as this will most probably require a lot of architectural design discussion. For that reason, my earlier approaches were all based on existing Form API functionality only.

chx’s picture

I like databind. Using it for entity forms blew my mind. As for #65, I thought so too but if you read the whole function it's being used.

effulgentsia’s picture

FileSize
21.93 KB

This re-adds tests from #52, slightly reworked for this direction, changes $form['#config'] to $form['#config_name'] and $element['#config'] to $element['#databind_options'], and changes form_databind_prepare_values($form, $form_state) to be called before the form's submit handlers with a comment in the patch as to why.

Didn't we also say further up that forms might want to mix and match config from several buckets? How would that work?

config_test.module in this patch answers this.

if we continue with this direction, then I doubt we'll be able to unblock the config conversion patches anytime soon, as this will most probably require a lot of architectural design discussion

If sun, chx, and I (3 of the FAPI maintainers) are ok with the FAPI changes, how much more architecture discussion is needed?

effulgentsia’s picture

Status: Needs review » Needs work
FileSize
21.3 KB
+++ b/core/modules/config/config.test
@@ -286,6 +286,95 @@ class ConfigFileContentTestCase extends WebTestBase {
+  /**
+   * - There is no direct mapping of $form structure keys to $config keys.
+   * - There is no direct mapping of input element names to $config keys.
+   * - There is a direct mapping of $form_state['values']['config'] to
+   *   configuration object names and keys within them.
+   * - Nevertheless, settings forms need to support all possible variations of
+   *   #tree and #parents.
+   * - Settings forms may be extended by other modules, but their values must
+   *   only be written to their module-specific configuration objects.
+   * - Saving a settings form without additional input updates the
+   *   configuration, but does not change any values.
+   */

This patch removes this outdated doc that was copy/pasted from #52.

effulgentsia’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Note that system_performance_settings() in HEAD differs from forms that call system_settings_form() in the following:
- It adds its own submit button (with a value/label that is not "Submit"), rather than having it added by system_settings_form().
- It is not themed with theme_system_settings_form().

#69 does not change this, but how to handle this might be something we want to decide on before converting all settings forms. I like that #69 decouples the existence of form elements databound to config storage from the above two things. But, if we want to preserve these two things for DX, we may want to preserve something like a system_settings_form() wrapper function for just that purpose.

joachim’s picture

> I like databind.

We tend not to make up works or splodge words together. So 'data_bind' at least. 'bound_data' better, perhaps?

Jose Reyero’s picture

Related though not the same, see #1648930: Introduce configuration schema and use for translation

That could be used to eventually autogenerate most settings forms on the fly, still we'd need this 'generic settings forms handling' in the first place.

chx’s picture

FileSize
9.93 KB

This patch adds 8 LoC to form.inc, 11 to system.module -- and it works. I converted aggregator for show.

Status: Needs review » Needs work

The last submitted patch, 1324618_74_loadables.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.03 KB

OK, OK, temporarily wrapped in if (isset($form['#config'])) {

gdd’s picture

Status: Needs review » Needs work

One thing this doesn't support is the ability to save data from a single form into multiple files, which we need for situations where modules form_alter() settings into another module's settings form. Talked to chx about this, and he is going to add the ability to override the main '#config' form variable on an element by element basis.

We also might want to bikeshed the naming a bit, '#load' isn't completely obvious. I'm not immediately coming up with anything better though.

If I read this patch right, these additions to Form API are only valid for system_settings_form(), which bothers me a bit. Overall thought I still like this patch, because it gets a solution into core and we can revisit the more wide-ranging databind solution later if we have time.

gdd’s picture

I've also asked effulgentsia to comment here

chx’s picture

Status: Needs work » Needs review
FileSize
40.17 KB

I do not expect this one to pass but it's quite a progress. It changes every form I was able to find, so far.

Status: Needs review » Needs work

The last submitted patch, 1324618_77.patch, failed testing.

sun’s picture

I still need to review the new proposal, but after getting an in-depth explanation of Symfony's Form component, I'm slowly leaning towards deferring any kind of data binding functionality in forms to D9 (+ Symfony Forms). Ideally, we'd do that for D8 already, but time won't permit that.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
40.48 KB

This is just a reroll of #79. I haven't reviewed it yet. Doing so now.

Status: Needs review » Needs work

The last submitted patch, 1324618_82.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
21.44 KB
40.04 KB

A bunch of cleanup in the specific forms, but no comment yet about the form.inc and system.module changes.

effulgentsia’s picture

FileSize
6.87 KB
46.99 KB

Updated user_admin_settings().

Status: Needs review » Needs work

The last submitted patch, 1324618_85.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
47.98 KB

This should get the tests to pass, but I have an idea for how to simplify that I will work on next.

chx’s picture

Assigned: sun » effulgentsia

Thanks so much for taking this issue! It's really important and I havent had the time to go after the failures (the syntax error was obvious but there were too many invidual fails).

I see you re-added + if (isset($form_state['loadables']['config'])) { this safeguard. Why? system_config_form() adds + $form_state['callbacks']['load']['config'] and surely there'll be a #load => config in a form that calls system_config_form. The reason I nuked it because it actually signals when we are finished converting.

effulgentsia’s picture

So my "simple" idea didn't pan out, because PHP doesn't allow for serialization of closures, so we can't use closures in $form or $form_state or they'll break when form caching is enabled (e.g., if a module adds an AJAX widget to the form).

I don't think #87 is a good solution: it adds $element['#load'] and $form_state['callbacks'] concepts to FAPI, which while fewer lines of code than #69, is still basically a databinding implementation that we'd need to name, document, etc. We could potentially get it sufficiently refined and documented, but is it worth it? It only works in the special case that no processing of $config->get() is needed, whereas as #87 shows, we have several places where that is needed (e.g., implode("\n") or drupal_get_path_alias()), causing '#load' to not work and thereby also re-requiring explicit saving in FORM_ID_submit(). This is what I was hoping to fix with the addition of closures in $element, but alas, that's incompatible with form caching.

Meanwhile, is requiring explicit $config->save() in FORM_ID_submit() so bad? I don't think so, so downgrading this issue to normal.

effulgentsia’s picture

Title: Re-implement system_settings_form() for the configuration system » Implement automated config saving for simple settings forms
Assigned: effulgentsia » sun
Category: task » feature
Priority: Major » Normal

surely there'll be a #load => config in a form that calls system_config_form

Not necessarily. I took it out of search_admin_settings(), because search_admin_settings_submit() does some interesting stuff, like include a "Reset to defaults" button.

sun’s picture

Meanwhile, is requiring explicit $config->save() in FORM_ID_submit() so bad? I don't think so

I agree.

There's not only this new semi data binding concept of #load and the special processing in system_config_form_submit() that would have to be documented and explained.

There's also the entire problem space of #1653026: [META] Use properly typed values in module configuration - which is not being accounted for in this data binding concept here yet; i.e., the binding should actually know i/o data types of the data being processed, so we can store an Boolean TRUE instead of a string that contains an integer '1'.

All of this essentially translates into transforming form input into values that are compatible with data types, as well as (re-)mapping data values to be compatible with properties as well as form/widget elements; i.e., DataTransformation and DataMapping.

Given that we've converted most settings forms already (see #1696224: Remove system_settings_form()) and given that forms for Configurables will go through the entity system (which has a very [too] simple data binding concept already [there's hope the new EntityFormControllers will improve it]), I doubt it really makes sense to attempt to introduce this concept for D8.

Instead of doing this, I'd rather suggest to target a full Form API replacement with Symfony's Form component for D9 (which happens to have all of this functionality built-in). Fact is - and I've already raised this concern to many others during DrupalCon Munich - after learning about the Form component, it seems clear that we're sorta re-inventing the wheel with many things we're currently doing for D8; most of Property/MetaData API is about describing "properties" so we can "understand" and map (and transform) data to them, and obviously, if you look at the entire Field API architecture, what you see is an enormously complex data binding/transformation concept that could be simplified a lot. We can experiment with that during D8 already (and I'd very open to re-purpose the Form project for doing so).

If we go with this simplified route, then the only remaining task for D8 would be to create a second, full stack of novice issues to change the $form definitions of settings forms to use form array keys to be more in line with the actual config object keys. (The $form structure was left untouched in all of the conversion issues due to this issue here.) If we choose to do so, then some of us should discuss and do 1-2 $form conversions first, in order to create a new tutorial (or extending the existing tutorial) to explain how to properly convert $form arrays.

gdd’s picture

To make it clear, the upshot of this would be that we don't convert system_settings_form() at all, and we continue to have explicit submit handlers for every form, correct?

sun’s picture

Yes, the simplified plan would be:

1) system_config_form() + system_config_form_submit() would remain as-is in HEAD (BC break intended).

2) mymodule_settings_form_submit() needs to care for saving the submitted form values appropriately (as with any other form).

3) system_config_form() exists as helper to standardize on form actions and success/confirmation message.

4) system_settings_form() & Co are scheduled for deletion as soon as there's a "variable tombstone".

sun’s picture

Assigned: sun » Unassigned
Issue tags: -Needs issue summary update
chx’s picture

I can live with this. (Just a note reading the comments: confgiurables are not entities.)

effulgentsia’s picture

confgiurables are not entities

@chx and anyone else interested in that discussion, please see #1761040: Rename Storable, Entity, and Configurable to Entity, ContentEntity, and ConfigEntity

sun’s picture

I'd like to see a final confirmation (or disagreement) from at least the following people before moving forward:

@beejeebus, @Jose Reyero, @alexpott, and lastly a @heyrocker hammer

gdd’s picture

I'm somewhat concerned about removing a functionality as widely used as system_settings_form(), but ultimately I think it makes sense. As ksenzee pointed out in #1324618-31: Implement automated config saving for simple settings forms, we've already moved to a situation where module developers have to think more about their data storage and mapping that back to the simplicity of system_settings_form() doesn't necessarily make a lot of sense and as we've seen from the varied implementation attempts above, is difficult.

So +1 from me.

alexpott’s picture

Heyrocker's hammer has fallen :)... but +1 from me too...

Because system_settings_form() was so easy it made the variables table a dumping ground and forcing module developers to think about how to save config will end up with a big win in the long run.

sun’s picture

Status: Needs review » Closed (won't fix)

OK, let's move forward with #91 then: I've created an initial proposal for the new (novice) conversion guidelines:

#1770830: [meta] Update module settings $form arrays

Let's discuss over there.

(The proper status would probably rather be to postpone this for D9, but there's no 9.x-dev yet, and the entire discussion might as well look completely different then...)

sun’s picture

Issue summary: View changes

Updated issue summary.