Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tags being deleted by any batch action and by drap and drop reordering of records #18328

Merged
merged 10 commits into from Oct 23, 2017

Conversation

@ggppdk
Copy link
Contributor

@ggppdk ggppdk commented Oct 13, 2017

Pull Request for Issues #18317 and #18086 and #18057

Summary of Changes

The tags are not retrieved properly by the code of AdminModel class
(by the batch actions methods and saveorder() method)

This is to due wrongly calculated 'tableClassName'
(when the descendant of this class is namespaced ?)
which results in empty UCM type & typeAlias, thus tags do not get loaded

There was common code (same code) called in 6 places

  • batch() method
  • 4 batch*() methods
  • saveorder() method

Fixed the code and moved it into a new method , initBatch()
(saveorder() needed some more fixing that the others)

Testing Instructions

Use batch actions and manual reordering of records

Expected result

The above task work properly
and they also do not delete the tags assigned to the articles

Actual result

The tags are deleted

Documentation Changes Required

initBatch() method added

Should this be protected or private ?

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 13, 2017

This is nasty bug, maybe a 3.8.2 milestone ?

Loading

$this->type = $this->contentType->getTypeByTable($this->tableClassName)
?: $this->contentType->getTypeByAlias($this->typeAlias);

// Get get tabs observer
Copy link
Contributor

@Quy Quy Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove duplicate get

Loading

@ghost
Copy link

@ghost ghost commented Oct 13, 2017

I have tested this item successfully on caf8b82


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@ghost
Copy link

@ghost ghost commented Oct 13, 2017

@ggppdk i guess this PR works only for com_content (not joomla-extensions/weblinks#368)

Loading

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Oct 13, 2017

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 13, 2017

i guess this PR works only for com_content (not joomla-extensions/weblinks#368)

@franz-wohlkoenig

This PR is a valid fix for all classes that extend ModelAdmin and face this issue,
I have tested just now with weblinks 3.7.0-beta1 and it is working

Loading

@@ -631,7 +590,7 @@ protected function batchTag($value, $pks, $contexts)
/**
* @var \JTableObserverTags $tagsObserver
*/
$tagsObserver = $table->getObserverOfClass('\JTableObserverTags');
$tagsObserver = $table->getObserverOfClass('Joomla\CMS\Table\Observer\Tags');;
Copy link
Contributor

@alikon alikon Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove double semicolon

Loading

@alikon
Copy link
Contributor

@alikon alikon commented Oct 13, 2017

I have tested this item successfully on caf8b82


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@alikon
Copy link
Contributor

@alikon alikon commented Oct 13, 2017

but i'm afraid we still have some issues about classname and TagsObserver

old $tagsObserver = $table->getObserverOfClass('\JTableObserverTags');
new $tagsObserver = $table->getObserverOfClass('Joomla\CMS\Table\Observer\Tags');

that is supposed to be fixed by #17739

Loading

@ghost
Copy link

@ghost ghost commented Oct 13, 2017

@alikon is this PR ready for RTC?

Loading

@alikon
Copy link
Contributor

@alikon alikon commented Oct 13, 2017

i would say yes cause it fix a big issue
but i suspect that there is still some issue with #18328 (comment) under the hood

so i'm afraid we need some feedback from mantainers
perhaps setting RTC gain more priority

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 13, 2017

@alikon

yes it was supposed to have been fixed, as you said above,
but it seems that fix was incomplete

foreach (JLoader::getDeprecatedAliases() as $alias)

https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/observer/updater.php#L63

will return 'jtableobservertags' which is not equal to 'JTableObserverTags'

thus this does not work

$this->table->getObserverOfClass('\JTableObserverTags');

(the slash is not the issue, lowercase comparison to uppercase is the issue)

Loading

@alikon
Copy link
Contributor

@alikon alikon commented Oct 13, 2017

@ggppdk maybe this one #17834 unvalidate #17739

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 13, 2017

The fixing of loading of classes via alias, regardless of letter case of the alias
will not effect this PR, and it should be done seperately of this PR

In this PR we just use the full qualified name of the class

This PR can be RTC

Loading

@ghost
Copy link

@ghost ghost commented Oct 13, 2017

RTC after two successful tests.

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 14, 2017

@mbabker , @wilsonge

This is an important bug-fix of issues (of not uncommon usage) resulting in data loss (tags deleted)
please review for adding J3.8.2 milestone, and maybe reviewing the PR too

see brianteeman comment here also saying that this is urgent,
#18317 (comment)

aka if this, can be fixed properly then it should be in J3.8.2 ?

Loading

if (empty($this->batchSet))
{
$this->batchSet = true;
$this->user = \JFactory::getUser();
Copy link
Contributor

@mbabker mbabker Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New class member variables should be documented for this and other $this->foo assignments.

Loading

Copy link
Contributor Author

@ggppdk ggppdk Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, but the previous guy that originally added them, did not do it,

you mean add them at the top of the class, together with other member variables,
and document them similarly ?

Loading

Copy link
Contributor

@mbabker mbabker Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please.

Loading

Copy link
Contributor Author

@ggppdk ggppdk Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I ll document them later today,
also 2 of them are not really needed to be defined as member variables

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 15, 2017

Added 3 new member variables and documented them,

but now this needs retesting !, no longer RTC !

Loading

@csthomas
Copy link
Contributor

@csthomas csthomas commented Oct 16, 2017

May be you can use $this->table === null instead variable $this->batchInitialized === null?

Loading

@csthomas
Copy link
Contributor

@csthomas csthomas commented Oct 16, 2017

now i have renamed $this->batchSet to $this->batchInitialized
and removed $this->user and $this->tableClassName

is it a B/C break , should i restore them ?

They have not been documented, I do not think so

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 16, 2017

May be you can use $this->table === null instead variable $this->batchInitialized === null?

yes my thought too,

just if some derived class decides to set $this->table in the constructor, then initBatch will not initialize the other member variables because $this->table === null will not be true

should i do this change ?

Loading

@laoneo
Copy link
Member

@laoneo laoneo commented Oct 16, 2017

is it a B/C break , should i restore them ?

If they have been not private then it can be counted as a BC break.

Loading

@csthomas
Copy link
Contributor

@csthomas csthomas commented Oct 16, 2017

$this->user was used by protected function checkCategoryId(). As there can be more batch methods in subclasses then all old variables probably has to stay initialised as before, some of them could be mark as deprecated.

3rd party component may use one of that variable which is not documented above.
Does Joomla support B/C for such variables?

Loading

@mbabker
Copy link
Contributor

@mbabker mbabker commented Oct 16, 2017

$this->foo when not declared on a class object is treated as a public variable. So, there is a case to be made that removing the use of undeclared class variables counts as a B/C break because the variables were implicitly created through certain code paths.

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 16, 2017

i ll add them back , exactly as before ))

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 16, 2017

I have restored class member variables, now it should be B/C
Now it can be re-tested

Loading

$contentType = new \JUcmType;
$this->type = $contentType->getTypeByTable($this->tableClassName)
?: $contentType->getTypeByAlias($this->typeAlias);

Copy link
Contributor

@csthomas csthomas Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be strict I suggest to initialise $this->contentType as before.

It would be nice to deprecate a few class variables (contentType, tableClassName, user) that you do not need right now if everyone agrees.

Loading

@Patrickbmccabe
Copy link

@Patrickbmccabe Patrickbmccabe commented Oct 17, 2017

I have this issue. (Please forgive my ignorance) How do I take part in a test ?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@ghost
Copy link

@ghost ghost commented Oct 17, 2017

@Patrickbmccabe please mark your Test as successfully:

  • open Issue Tracker
  • Login with your github-Account
  • Click on blue "Test this"-Button above Authors-Picture
  • mark your Test as successfully
  • hit "submit test result"

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 17, 2017

Done i restored 1 more local variable as member property (as pointed out by @csthomas )

I retested too
Ready to be tested again

Loading

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 19, 2017

@Puckmeister
@franz-wohlkoenig
@rgblogs
@Pilotnik
@Fuchsritter
@cybersalt
@AndySDH
@Shazrina1994

you had reported having issues with the tags being lost

maybe you can test this PR

Loading

@ghost
Copy link

@ghost ghost commented Oct 19, 2017

I have tested this item successfully on 88fec66

Darg-and-Drop-Reordering works also as Batch-Action; Test on Articles.

Thanks @ggppdk


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@csthomas
Copy link
Contributor

@csthomas csthomas commented Oct 19, 2017

I have tested this item successfully on 88fec66

Tested #18317, #18086 and #18057


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@ghost
Copy link

@ghost ghost commented Oct 20, 2017

RTC after two successful tests.

Loading

@Pilotnik
Copy link

@Pilotnik Pilotnik commented Oct 23, 2017

It's OK now. Thanks.

Loading

@mbabker mbabker added this to the Joomla 3.8.2 milestone Oct 23, 2017
@mbabker mbabker merged commit 4b57947 into joomla:staging Oct 23, 2017
4 of 5 checks passed
Loading
@barraclm
Copy link

@barraclm barraclm commented Oct 24, 2017

I would love to test this, but I am but a humble user, one of thousands, probably tens or even hundreds of thousands who uses Joomla to manage web sites. It may be 'fixed' for geeks, but for us lesser mortals it remains a MAJOR bug until an update appears in the update channel. Does anyone know when we might expect this?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@Patrickbmccabe
Copy link

@Patrickbmccabe Patrickbmccabe commented Oct 24, 2017

I agree with Barraclm, I would love to give my time amd effort to this, but it is beyond my ability. This bug has caused major issue for a Library website I had created 100's of tags for.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18328.

Loading

@ghost
Copy link

@ghost ghost commented Oct 24, 2017

@barraclm https://github.com/joomla/joomla-cms/milestones - there you find expected Release Date.
@Patrickbmccabe https://docs.joomla.org/Bug_Tracking_Process - Information about Bug tracking using Patchtester.

Loading

@ggppdk ggppdk deleted the patch-51 branch Oct 24, 2017
@barraclm
Copy link

@barraclm barraclm commented Oct 24, 2017

Thank you Franz. That is very helpful.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet