? Success

User tests: Successful: Unsuccessful:

avatar elinw
elinw
6 Oct 2013
avatar elinw elinw - open - 6 Oct 2013
avatar brianteeman
brianteeman - comment - 13 Oct 2013

Please can you create a corresponding item on joomlacode - thanks

avatar garyamort
garyamort - comment - 21 Oct 2013

libraries/legacy/model/admin.php has 2 errors in saveorder:
$table must be set at the start of the method, otherwise it generates a PHP error
the call to createTagsHelper must be scoped[ie $this->createTagsHelper], otherwise it generates a PHP error

With those 2 fixes, saveorder now 'works' in that the order is saved - but the tags are now lost. Previously, nothing was actually "saved", the ajax frontend just doesn't know how to handle errors.

I pulled elinw's branch into my repo and applied the fixes, as soon as I get it pushed back up I can submit a pull request for the elinw branch[which would then update this request?]

Time permitting I'll take a further look this evening.

avatar garyamort
garyamort - comment - 21 Oct 2013

Ok, pull request is completed here: elinw#100

Unsure of coding style: I used local variables in saveorder rather than relying on $this->batchSet as is done in batchAccess, batchCopy, batchLanguage, and batchMove: obviously it should be a consistent style, either all use the batchSet check, or all use local variables.

batchAccess,batchCopy,batchLanguage, and batchMove are also missing a declaration for $this->typeAlias...maybe it is set someplace else? Otherwise, it needs to be set as done in saveorder.

Finally, I noticed that JHelperTags and JTableObserverTags both contain protected properties $replaceTags so they can be flagged not to delete existing tags - but there is almost no way to set the property. Should this be a public property for now to at least make it available? Also there are a number of calls from the observer to the helper which don't fill in the $replace parameter - presumably it should almost always send it's own property.

avatar elinw
elinw - comment - 21 Oct 2013

Basically the replace property was added because we wanted to be able not
to replace when doing batch (in tagging a single item the save is treated
as a retagging i.e. there is replace similar to the way most of the other
jtablenested classes are handled e.g. user groups). I agree that the way
the observer is doing it is frustrating and it was one reason that I had to
handle fixing this in the way I did. It's not a bad idea at all to make
it publiic and that would increase the flexibility of tagging, although I
think that the option has to be handled with care.

On Mon, Oct 21, 2013 at 2:39 AM, Gary A Mort notifications@github.comwrote:

Ok, pull request is completed here: elinw#100elinw#100

Unsure of coding style: I used local variables in saveorder rather than
relying on $this->batchSet as is done in batchAccess, batchCopy,
batchLanguage, and batchMove: obviously it should be a consistent style,
either all use the batchSet check, or all use local variables.

batchAccess,batchCopy,batchLanguage, and batchMove are also missing a
declaration for $this->typeAlias...maybe it is set someplace else?
Otherwise, it needs to be set as done in saveorder.

Finally, I noticed that JHelperTags and JTableObserverTags both contain
protected properties $replaceTags so they can be flagged not to delete
existing tags - but there is almost no way to set the property. Should this
be a public property for now to at least make it available? Also there are
a number of calls from the observer to the helper which don't fill in the
$replace parameter - presumably it should almost always send it's own
property.


Reply to this email directly or view it on GitHub#2157 (comment)
.

avatar garyamort
garyamort - comment - 22 Oct 2013

Added a new pull request for elin's branch to: 1: make sure typeAlias is set in the batch methods if it was not[should not happen...but] and 2: modified my code to bring it in line with the same style[replaced $this with static]

Add a Comment

Login with GitHub to post a comment