akeeba / fof

Rapid Application Development framework for Joomla!™ 3 and 4
0 stars 0 forks source link

Multiple model form fields show same options (incorrect data ignoring key_field) #582

Closed willfaulds closed 8 years ago

willfaulds commented 8 years ago

Using latest FOF3 build from dev branch "rev1EA560E-1454523712" & clean J3.4.8

Multiple model form fields in an input form form.form.xml all show the 1st instance's data and not that exepcted according to each instance's key_field.

I have found that the case on the model attribute effects results. It appears to only effect multiple instances accessing the same table.

i.e. 3 models using model="products" all show the data from the 1st instance's key_field. However if the 2nd instance uses model="Products"it shows the correct data for it's key_field...

nikosdion commented 8 years ago

You can't have more than one Model field referencing the same Model on a single form. The reason is the $loadedOptions static cache. As you can see in FOF30\Form\Field\Model::getOptions the cache key generated comes from the name of the form and the model attribute. If we remove the options cache we have a tremendous performance impact (100s of queries, 10x-40x page load time increase).

willfaulds commented 8 years ago

Ah OK - understood - I'll add a note to the Model wiki page

willfaulds commented 8 years ago

Although thinking about it - as it worked with different cases would I be apple to append the model table name to force around caching?

e.g. model="products?1" or similar?

nikosdion commented 8 years ago

Hm, we could possible add an attribute to index each model, but what is the use case?

willfaulds commented 8 years ago

Let's leave it for now as I'm still getting to grips with FOF3 and my use case is not fully thought through yet. As an aside - should slug be on the list of MagicFields?

nikosdion commented 8 years ago

No, slug is not a magic field. All the magic fields have special meaning and are used throughout the Model code. For example, when you are publishing/unpublishing records the enabled field is modified. Same for created, modified and locked when you're creating / locking / modifying a record. You get the idea. The slug field is a convention, it doesn't have a special meaning in our code.

willfaulds commented 8 years ago

ok my mistake on what's called what - is there a list of convention fields?

nikosdion commented 8 years ago

Not really, but we tend to use the following:

willfaulds commented 8 years ago

Thanks again Nik, I'm sure it feels like explaining the obvious ;)

So here's my concept (explains why I attempted multiple model="products")

I have products which have categories and nested sub categories and nested sub-sub categories... I'd like to achieve a field type (or group see below) which achieves a drilldown select - here's a simple working jquery plugin which achieves roughly what I'm thinking. Each select is column.

I can conceive this not being too bad with multiple field types following a strict naming convention & reasonably simply javascript. The first field type ModelDrilldownMaster would pull ALL the possible options for all columns (an amended version of Model) and inject some javascript (containing the data) which injects and controls the other inputs. The other inputs would be created from another field type ModelDrilldownSlave.

Sound like a sensible approach (or can a field addKnownField meaning I'd only need one field type)?

nikosdion commented 8 years ago

I am not convinced that the Model field makes sense in this case, or even that XML forms make sense at all.

willfaulds commented 8 years ago

I've dug deeper and now understand the TreeModel (fantastic) - if I were to commit to building a Tree field - the frontend aspects (js) I can handle - basic GUI to allow tree placement perhaps with this

Again I come to wondering whether I can achieve this with a field type alone... it will need to call functions on save() e.g. insertAsChildOf to update the whole table's lft and rgt- getting a field edit anything other than its column/cell... Could I extend another class from within the field type file? I know this wouldn't follow the code placement conventions but I can see the advantages of quick deployment through a single file approach... For this exact scenario I suppose it could easily be done with a new magic field.

For the time being I can concentrate on getting it working and then as a last resort package it in a plugin?

nikosdion commented 8 years ago

I don't think that having to enter the lft and rgt makes any sense – these are implementation details. You can have two sites with the same tree structure and different lft/rgt values (think about what happens on record deletion). You only need the ID of the root node and how many levels deep you want to go. But, as I said, a field is only a view element. You want to add some kind of Model level functionality which doesn't belong to a field. I'm not sure how you could implement that without writing code for a specific use case.

willfaulds commented 8 years ago

I've updated my last comment quite a bit (bad habit of editing after posting) but I don't understand with out lft & rgt values how in an item edit view you could assign the exact locations of a node within the tree?

willfaulds commented 8 years ago

I think I've got the basics working here (manual input of lft&rgt in the Category form) https://github.com/willfaulds/todo-fof-example/blob/with-fof-3-extended/component/backend/Model/Categories.php

Support creation of new but now I need to support rearranging too.

willfaulds commented 8 years ago

Hey Nik, having played much more I think it would be very useful to expand the TreeModel->getNestedList() or add a new more comprehensive function;

Without lft&rgt values we cannot safely use the generated list to rearrange nodes - if a node was not accessible(ACL) - we won't know either lft&rgt values for node's new/end position (the start position is not a problem as we can select the node, before moving it, another way).

Without lft&rgt values we cannot (certainly I can't ;p ) easily/quickly work out what the status of a node is (e.g. isLeaf). Level/depth calcs will also be potentially flawed without unique separators as we are limited to count their occurrence.

I have built a basic TreeSelect here which shows where this is coming from.

nikosdion commented 8 years ago

Considering that we're doing a raw SQL query in getNestedList I don't see your point.

willfaulds commented 8 years ago

I will explain with more fully formed code later ;)

willfaulds commented 8 years ago

Hi again Nik,

So I found some time to progress with working the TreeModel into com_todo and its working great - I've actually upgraded com_todo to use the XML database schema install a TreeModel table with a root node, added a TreeSelect list to the edit view which allows setting the node's parent etc... this is all working great.

Next stage I want to expand the TreeSelect's usefulness it gets a list of nodes so can be used to assign nodes to items from another table (this works fine as is) but I want to be able to disable all nodes except leaf nodes in certain instances...

I think this leaves 2 options add support for getting a more comprehensive node list from TreeModel or handle it elsewhere. Personally I think it makes sense to be a feature of TreeModel whether an amendment to getNestedList, or a new function, I don't know (see working example below which maintains all backwards compatibility)

public function getNestedList($column = 'title', $key = null, $seperator = '  ', $comprehensive = false)
{
  $db = $this->getDbo();

  $fldLft = $db->qn($this->getFieldAlias('lft'));
  $fldRgt = $db->qn($this->getFieldAlias('rgt'));

  if (empty($key) || !$this->hasField($key))
  {
    $key = $this->getIdFieldName();
  }

  if (empty($column))
  {
    $column = 'title';
  }

  $fldKey = $db->qn($this->getFieldAlias($key));
  $fldColumn = $db->qn($this->getFieldAlias($column));

  $query = $db->getQuery(true)
    ->select(array(
      $db->qn('node') . '.' . $fldKey,
      $db->qn('node') . '.' . $fldColumn,
      $db->qn('node') . '.' . $fldLft,
      $db->qn('node') . '.' . $fldRgt,
      '(COUNT(' . $db->qn('parent') . '.' . $fldKey . ') - 1) AS ' . $db->qn('depth')
    ))
    ->from($db->qn($this->tableName) . ' AS ' . $db->qn('node'))
    ->join('CROSS', $db->qn($this->tableName) . ' AS ' . $db->qn('parent'))
    ->where($db->qn('node') . '.' . $fldLft . ' >= ' . $db->qn('parent') . '.' . $fldLft)
    ->where($db->qn('node') . '.' . $fldLft . ' <= ' . $db->qn('parent') . '.' . $fldRgt)
    ->group($db->qn('node') . '.' . $fldLft)
    ->order($db->qn('node') . '.' . $fldLft . ' ASC');

  $tempResults = $db->setQuery($query)->loadAssocList();

  $ret = array();

  if (!empty($tempResults))
  {
    foreach ($tempResults as $row)
    {
      if($comprehensive)
      {
        $ret[$row[$key]] = array(
          $column => $row[$column],
          'depth'=> $row['depth'],
          'lft'=> $row[$this->getFieldAlias('lft')],
          'rgt'=> $row[$this->getFieldAlias('rgt')],
        );
      }else {
        $ret[$row[$key]] = str_repeat($seperator, $row['depth']) . $row[$column];
      }
    }
  }

  return $ret;
}
nikosdion commented 8 years ago

I don't see how the output of getNestedList can be used by JHtml to create an HTML <select > drop-down when your proposed $comprehensive parameter is set to true. What you're trying to do seems similar to getting the descendant nodes which is already possible with getDescendants.

willfaulds commented 8 years ago

Thanks Nik,

I'm not great at seeing where other solutions are but that has pointed me in the right direction: $model->getRoot()->getDescendantsAndSelf()->toArray()

I can iterate over that and prepare a list however I want for JHtml :)

nikosdion commented 8 years ago

There you go :)