? ? Pending

User tests: Successful: Unsuccessful:

avatar richard67
richard67
26 Oct 2019

Pull Request for Issue #26828 .

Summary of Changes

Remove the check for featured_up and featured_down times in the content table class.

The featured_up and featured_down columns have been added to database table #__content_frontpage with PR #25979 , thanks @eshiol for that new functionality and your patience.

During development, the new database columns first were added to table #__content and later have been mode to table #__content_frontpage.

This PR here removes a remainder of that change which causes the issue described in #26828 .

Thanks @Quy for having discovered the issue.

Testing Instructions

  1. Set php error reporting to E_ALL in your php.ini and switch on error logging to file or display of errors.
  2. Make a fresh installation of a clean, current 4.0-dev branch.
  3. When loging in to admin (backend) for the very first time, watch the PHP error log / error display.

Expected result

No errors or warnings or notices.

Actual result

PHP Notice: Undefined property: Joomla\Component\Content\Administrator\Table\ArticleTable::$featured_up in \libraries\src\Table\Content.php on line 278
PHP Notice: Undefined index: featured_up in \administrator\components\com_content\Model\ArticleModel.php on line 950
PHP Notice: Undefined index: featured_down in \administrator\components\com_content\Model\ArticleModel.php on line 950

Documentation Changes Required

None.

avatar richard67 richard67 - open - 26 Oct 2019
avatar richard67 richard67 - change - 26 Oct 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Oct 2019
Category Libraries
avatar richard67 richard67 - change - 26 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 26 Oct 2019
avatar Quy
Quy - comment - 26 Oct 2019

This is still an issue.

PHP Notice: Undefined property: Joomla\Component\Content\Administrator\Table\ArticleTable::$featured_up in \libraries\src\Table\Content.php on line 278

avatar richard67 richard67 - change - 26 Oct 2019
Title
[4.09] Fix featured up and down null date checks in content table
[4.0] Fix featured up and down null date checks in content table
avatar richard67 richard67 - edited - 26 Oct 2019
avatar Quy
Quy - comment - 26 Oct 2019

Set to null if not set $this->featured_up and $this->featured_down.

avatar richard67
richard67 - comment - 26 Oct 2019

@Quy Ah it has been moved once from the #__content to the #__content_frontpage table!!! Here in the code for the content table it just has to be removed, I think. Will check.

avatar richard67
richard67 - comment - 26 Oct 2019

Or maybe saving to the #__content_frontpage is missing and has to be added?

avatar richard67 richard67 - change - 26 Oct 2019
Labels Added: ?
avatar richard67
richard67 - comment - 26 Oct 2019

@Quy You can test with my latest commit but I am not sure if that is right. I think the Content.php is only for the #__content table.

avatar richard67
richard67 - comment - 26 Oct 2019

I've pinged George with a comment here, hope we will reply: #25979 (comment)

avatar Quy
Quy - comment - 26 Oct 2019

Notices are gone, however, featured dates don't swap when finish date is less than start date.

avatar richard67
richard67 - comment - 26 Oct 2019

Notices are gone, however, featured dates don't swap when finish date is less than start date.

I think this is the wrong fix then.

avatar richard67
richard67 - comment - 26 Oct 2019

@Quy It has been moved from the content table to the content frontpage table during development of that and there has been something forgotten for saving, that's what I think. Or can you check if changes on these dates are saved so you later see them when going back to the form from somewhere else?

avatar Quy
Quy - comment - 26 Oct 2019

Are you suggesting that lines 277 to 284 should not be there at all?

avatar Quy
Quy - comment - 26 Oct 2019

I do see them when editing.

avatar richard67
richard67 - comment - 26 Oct 2019

Yes, that's what I think. You can save changes in them? Gotta go now, brb later.

avatar Quy
Quy - comment - 26 Oct 2019

Yes changes are saved.

avatar richard67
richard67 - comment - 26 Oct 2019

And if you remove the code for those columns completely? Changed still saved?

avatar richard67
richard67 - comment - 26 Oct 2019

Am away from desk now, can’t test myself.

avatar Quy
Quy - comment - 26 Oct 2019

Yes.

avatar richard67
richard67 - comment - 26 Oct 2019

I think I've seen in changes of that PR that changes on these 2 new columns are saved by event trigger. So I think they should be removed here. @wilsonge If you find some time, please check and comment.

avatar richard67
richard67 - comment - 26 Oct 2019

@Quy And if we remove the complete function load(), too?

avatar richard67
richard67 - comment - 26 Oct 2019

Hmm, I see, the function is necessary to fill times in the edit form.

avatar richard67
richard67 - comment - 26 Oct 2019

@Quy Now it should be ok. Please test.

avatar richard67
richard67 - comment - 26 Oct 2019

@eshiol Could you check and review and maybe test, too?

avatar richard67 richard67 - change - 26 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 26 Oct 2019
avatar SharkyKZ
SharkyKZ - comment - 26 Oct 2019

The code for loading data for display should be moved to article model's getItem() method. Date sanity checks should be moved to model's featured() method and maybe to Joomla\Component\Content\Administrator\Table\FeaturedTable class. We don't really use the latter when saving though.

avatar richard67
richard67 - comment - 26 Oct 2019

@SharkyKZ I see. Will do that later.

avatar richard67
richard67 - comment - 26 Oct 2019

@SharkyKZ

The code for loading data for display should be moved to article model's getItem() method.

Will that work for frontend editing, too?

avatar SharkyKZ
SharkyKZ - comment - 26 Oct 2019

No, that will have to be done in the frontend model as well.

Although I might be completely wrong with this suggestion. To me it seems that table class shouldn't load properties from other tables. But then Joomla\CMS\Table\User does exactly that with groups. So I'm not sure anymore.

avatar richard67
richard67 - comment - 26 Oct 2019

@SharkyKZ I get an error 1054 Unknown column 'featured_up, featured_down' in 'field list' when testing your suggested change and opening the article for edit. The swapping of the dates is rubbish anyway, I'll leave that away. Other sanity checks seem already to be done.

avatar richard67 richard67 - change - 26 Oct 2019
The description was changed
avatar richard67 richard67 - edited - 26 Oct 2019
avatar richard67
richard67 - comment - 26 Oct 2019

@SharkyKZ @Quy Please test. Main thing is the PHP notices go away and all works well. Architectural improvements should be considered for future PR's.

avatar Quy Quy - test_item - 26 Oct 2019 - Tested successfully
avatar Quy
Quy - comment - 26 Oct 2019

I have tested this item successfully on f8557aa


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

avatar richard67
richard67 - comment - 26 Oct 2019

@infograf768 Could you test this here, too?

avatar brianteeman brianteeman - test_item - 26 Oct 2019 - Tested successfully
avatar brianteeman
brianteeman - comment - 26 Oct 2019

I have tested this item successfully on f8557aa

tested with the json api


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

avatar SharkyKZ SharkyKZ - change - 26 Oct 2019
Status Pending Ready to Commit
avatar SharkyKZ
SharkyKZ - comment - 26 Oct 2019

RTC


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

avatar infograf768 infograf768 - change - 27 Oct 2019
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-10-27 05:33:44
Closed_By infograf768
Labels Added: ?
avatar infograf768 infograf768 - close - 27 Oct 2019
avatar infograf768 infograf768 - merge - 27 Oct 2019
avatar infograf768
infograf768 - comment - 27 Oct 2019

tks

avatar richard67
richard67 - comment - 27 Oct 2019

tested with the json api

@brianteeman Was your test maybe for #26825 and not this one here? Just to be sure all is right.

avatar brianteeman
brianteeman - comment - 27 Oct 2019

my test was for here

Add a Comment

Login with GitHub to post a comment