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

Clean up the published method. #6893

Closed
wants to merge 4 commits into from

Conversation

roland-d
Copy link
Contributor

@roland-d roland-d commented May 5, 2015

Goal

The goal of this pull request is to clean up some code that I found is duplicate and could be improved after checking #3189. This completely removes the publish() override for the JTableContent class as it is obsolete with the update.

How to test

  1. Go to the article manager:
    administrator/index.php?option=com_content&view=articles
  2. Unpublish an article and check that it works
  3. Publish an article and check that it works
  4. Apply this pull request
  5. Unpublish an article and check that it works
  6. Publish an article and check that it works

@infograf768
Copy link
Member

Looks like Travis is not happy.

@roland-d
Copy link
Contributor Author

roland-d commented May 5, 2015

@infograf768 For good reason Travis wasn't happy, there was an issue indeed but is solved now. Travis is happy 😄


if (!is_array($pks))
{
$pks = array($pks);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think type casting is not needed here. It will causes incorrect behavior in case we call publish method of a JTable object (without passing $pks variable, just want to publish the current loaded record).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be typecasting to an array because the code expects an array as is mentioned in the docblock.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be null (If not set the instance property value is used). Please see this sample code:

require_once JPATH_ADMINISTRATOR.'/components/com_contact/tables/contact.php';
$row = JTable::getInstance('Contact', 'ContactTable');
$row->load(1);
$row->publish();

In this case, the value of $pks value is null, and the current record should be published (It doesn't work now because of a typo in the class and I made a separate PR to fix it).

@alataknet
Copy link

@test success i published and unpublished an article successfully


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

@Dylis
Copy link

Dylis commented May 9, 2015

@test - Success. Patch tested with article set both to registred and public display.


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

@zero-24
Copy link
Contributor

zero-24 commented May 9, 2015

RTC. Thanks!


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

@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label May 9, 2015
@wilsonge wilsonge closed this in e5e4d36 May 10, 2015
@wilsonge
Copy link
Contributor

Merged - Thanks!

@wilsonge wilsonge added this to the Joomla! 3.4.2 milestone May 10, 2015
@joomdonation
Copy link
Contributor

Hmm. This PR is still having issue which I pointed above. I don't think it is not good to be merged yet :(.

@roland-d
Copy link
Contributor Author

@joomdonation Correct, I will create a new PR for that.

johanjanssens pushed a commit to joomlatools/joomla-fork that referenced this pull request Jun 19, 2015
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
@roland-d roland-d deleted the cleanup-content branch April 13, 2016 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants