Remove call to non-existing method createTagsHelper() , as its work is now handled by events

Correct

Looks good on code review can we just have a quick user test here please

changed Title to show its about 4.0.

I have tested this item ? unsuccessfully on 00a5f06

Click first Article of 4: User can move Article only after last Article - same Behaviour as without PR.

@franz-wohlkoenig

Are you getting some error (open browser console and go to network Tab)
or you cannot drag the row with the mouse at the position that you want ?

No Errors on Console- and Network-Tab.

Cannot drag the Row where i want, its only possible at end of all Articles. Example: Drag first Article below 4th works, between 2nd and 3rd didn't work.

Drag first Article below 4th works, between 2nd and 3rd didn't work.

That's simply because you can rearrange the article order per category. It's not a visual rearrangement, it's functional. Try to select a category first and try drag and drop again...

Cannot drag the Row where i want

I see, thanks for testing

this PR does not try to fix the difficult drag and drop JS / CSS issue
we are only fixing the server side errors if someone manages to drag and drop the row at a new place

so if you manager to drop at any place and then you refresh the page and you see it at the new place then this PR works

@dgrammatiko

you are right, i did not think this case now
i was talking about difficult drag when dragging the row handle a little out of table,

I have tested this item ? unsuccessfully on 00a5f06

1. Drag first of four Articles on last Position, so its th 4th.
2. Click on Submenu "Article" to Refresh Page
3. Dragged Article s moved from 4th to second Place in Table.

Edit: Test using 4 Articles of same Category.

@dgrammatiko thanks, haven't thougt about this.

I have tested this item ✅ successfully on 00a5f06

Works: using 4 Articles of same Category (had at last Test forgotten to apply PR).

I see another issue
that i think is not related to this PR

When you click to sort table by the ordering attribute
you get the articles listing to be ordered only by
'ordering' column
and not be
'catid, ordering' columns

it does not look related to this the PR,
as this PR does not touch the Model ordering stuff

OK Merging this with the good test. If there's a bug with sorting would you mind opening a fresh ticket?

OK Merging this with the good test. If there's a bug with sorting would you mind opening a fresh ticket?

ok thanks, I ll test locally why list is ordered like this and then i ll open issue or PR

@dgrammatiko
@wilsonge

a question please

i see that the in article manager the ordering when you click the ordering column is done

by: 'a.ordering'
and not by : 'a.catid, a.ordering'

ones need to filter by a single category
to be able to easily order a category

but the implementation of of AJAX reordering is able to handle multiple ordering groups in same page
and when you drag an item it highlights (J3) the edges of the ordering group, (in J4 there is some CSS issue it and the ordering borders are not highlighted)
so i am confused why the ordering is only by 'a.ordering'

some views take advantage of the ordering groups and do:
e.g. contacts

		if ($orderCol == 'a.ordering' || $orderCol == 'category_title')
		{
			$orderCol = $db->quoteName('c.title') . ' ' . $orderDirn . ', ' . $db->quoteName('a.ordering');
		}

e.g. the module manager takes advantage of the ordering groups and does:

		if ($listOrder === 'a.ordering')
		{
			$query->order($this->_db->quoteName('a.position') . ' ASC')
				->order($this->_db->quoteName($listOrder) . ' ' . $this->_db->escape($listDirn));
		}

i mean this is missing in article manager db model:

		if ($orderCol === 'a.ordering')
		{
			$query->order($this->_db->quoteName('a.catid') . ' ASC')
				->order($this->_db->quoteName($orderCol) . ' ' . $this->_db->escape($orderDirn));
		}
		else
		{
			$query->order($db->escape($orderCol) . ' ' . $db->escape($orderDirn));
		}

you only have the
$query->order($db->escape($orderCol) . ' ' . $db->escape($orderDirn));

but this grouping does not exist in articles manager (it does not exist in J3 either),
was it removed for some reason ?
or it was never there ?

It was never there and arbitrarily ordering articles by category ID is IMO an unwanted behavior. If you want a complex order condition then our UI should support it, but arbitrarily saying "if the user specifies they want to order on this column we're going to also include some other random columns in the order condition" is IMO wrong (because at the end of the day the model state is all based on a column name; if we had some type of named sort/ordering conditions independent of the column names that would be the appropriate place to introduce a complex multi-column ordering behavior).

If you want a complex order condition then our UI should support it, but arbitrarily saying "if the user specifies they want to order on this column we're going to also include some other random columns in the order condition" is IMO wrong

i understand your argument,
because i have ordering groups in my extension by category_id, language, "isvisible_state",
that is a total of 3 columns and i think overdid it

But about grouping by category i am confused in article manager
What confuses me is

1. on re-order action , the ordering value of records is incremented (in DB) according to category id so it is not arbitrary to sort by category id and then by 'a.ordering' when listing the records

2. Sort by ordering group first and then by 'a.ordering' exists in some other views , e.g. in contacts manager we are order by category and then by a.ordering, e.g. in module manager again we first order by the ordering group, which in that case it is the 'position'

3. the listing view itself uses the category id to create table rows like:
   

4. and also when you drag it highlights only rows of the group
   if you do not sort by ordering group first, then you get some records appearing in the previous and next pages

5. adding this DB articles model of J3

		if ($orderCol === 'a.ordering')
		{
				$query->order($db->escape('a.catid, a.ordering') . ' ' . $db->escape($orderDirn));
		}
		else
		{
			$query->order($db->escape($orderCol) . ' ' . $db->escape($orderDirn));
		}

and showing e.g. 100 items in the listing
i can reorder all shown categories without filtering the view by 1 category at a time
saving works as expected

on re-order action , the ordering value of records is incremented (in DB) according to category id so it is not arbitrary to sort by category id and then by 'a.ordering' when listing the records

Yes it is. Is there any awareness given to the user that the ordering is based on a complex condition (multiple columns)? Does the user expect that when sorting by ordering it is actually in order by category ID then by the ordering within a category?

Sort by ordering group first and then by 'a.ordering' exists in some other views , e.g. in contacts manager we are order by category and then by a.ordering, e.g. in module manager again we first order by the ordering group, which in that case it is the 'position'

That's still an arbitrary behavior at best. Because, again, the model state is based on a column name and the models are making an arbitrary decision to order by multiple columns when the state is given a specific single column to order by.

Does every use of every model (data type) that supports an ordering column also mandate that the model must order by an ordering group before applying the specified ordering column's order? Is this communicated in developer documentation, end user documentation, or in any way shape or form the expected user behavior?

the listing view itself uses the category id to create table rows

That is not an argument to couple a MODEL to a VIEW. How a view interprets and renders data to be consumed by a client (browser for HTML documents, JSON if core would actually support something other than HTML as a first class citizen) should not cause the model to have view specific behaviors and if there are view specific behaviors in a model that means the separation of concerns between the different components of MVC is broken.

and also when you drag it highlights only rows of the group
if you do not sort by ordering group first, then you get some records appearing in the previous and next pages

Again, this is a view behavior that has nothing to do with the data model.

ok, i understand your points
thanks for time taken to answer the question
since now i know that no PR is needed for changing the query