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

Added Serializable interface and methods to JTable. Fixes #16185. #16216

Closed
wants to merge 5 commits into from
Closed

Added Serializable interface and methods to JTable. Fixes #16185. #16216

wants to merge 5 commits into from

Conversation

Spudley
Copy link
Contributor

@Spudley Spudley commented May 23, 2017

Pull Request for Issue #16185 .

Summary of Changes

Added Serializable interface and methods to JTable. Excluding the DB object from serialization, because under some circumstances the DB object may contain closures; these are unserializable, and thus caused a crash when serializing JTable.

Testing Instructions

  • Install Joomla any of 3.7.0 / 3.7.1 / 3.7.2.
  • Install K2 extension.
  • Create a K2 category and add some articles to it.
  • Create a menu entry for the category view.
  • Navigate to the menu entry: It should work.
  • Turn on Joomla's debug mode.
  • Navigate to the menu entry again: It will crash with Serialization of 'Closure' is not allowed.
  • Now install the patch.
  • Keep debug mode enabled.
  • Navigate to the menu entry again: It should now work correctly.

Expected result

See above: The category view should work correctly in debug mode.

Actual result

Without the patch, navigating to the category view in debug mode crashes and gives the following error:

Exception
Serialization of 'Closure' is not allowed
.../libraries/joomla/cache/controller/callback.php:184

Documentation Changes Required

None.

@brianteeman
Copy link
Contributor

Please see the comments by @mbabker on this at #16185

@ghost
Copy link

ghost commented May 24, 2017

Comment of mbabker sounds like "no Core Issue" > close, @brianteeman?

@brianteeman
Copy link
Contributor

i would leave it to @mbabker or @rdeutz to make that decision

@rdeutz
Copy link
Contributor

rdeutz commented May 24, 2017

In some way this follows the advice Michale gave, but I have my doubts if we should cache table data this way. I might miss the point but what is when we cache the user table data, would that not allow to get the user data out of the cache and maybe misuse it. Maybe I am too paranoid :-)

@mbabker
Copy link
Contributor

mbabker commented May 24, 2017

Personally I think caching a database related object (JTable, JDatabaseDriver) is asking for trouble and you're better off caching just the record data. Unfortunately there isn't an eloquent way to get just the record data (get_object_vars() can work as long as you aren't running it from within the class and the only public properties are the ones created with the field structure).

The fix here is technically valid, anyone who is doing something resulting in the serialization of JTable objects avoids running into a scenario where the database driver gets included into the mix, and that's in general not a good object to serialize.

@Spudley
Copy link
Contributor Author

Spudley commented Jun 6, 2017

@mbabker, @rdeutz -- I'm not totally clear from your comments here whether you're falling in favour of this PR or against it? I'm hoping the fact that it hasn't been closed means you're still considering it? :)

@Kev1n337
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 82f9364

I wasn't able to reproduce this issue following these steps:

  • Installed Joomla 3.7.0
  • Installed K2 extension
  • Created K2 Categories
  • Created new items and attached them to the new category
  • Created menu item directing to the articles in this category
  • navigated to this view
  • It works
  • After enabling "Debug System" in Global configuration, it still works the same as before

@icampus


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

@brianteeman
Copy link
Contributor

Note. K2 had a lot of updates last week so "maybe" they changed something.

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@ghost
Copy link

ghost commented Jul 19, 2019

Note. K2 had a lot of updates last week so "maybe" they changed something.

@Spudley can you please at this?

@jwaisner
Copy link
Member

@Spudley are you able to update this PR if this is still something that needs resolved?

@jwaisner jwaisner added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Mar 11, 2020
@Spudley
Copy link
Contributor Author

Spudley commented Mar 11, 2020

@Spudley are you able to update this PR if this is still something that needs resolved?

Hi @jwaisner. I am no longer maintaining the Joomla system that was exhibiting the bug and it's been two years since I last thought about it, so I'm not really in a position to follow it up any longer. Thank you.

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @jwaisner by The JTracker Application at issues.joomla.org/joomla-cms/16216

@joomla-cms-bot joomla-cms-bot removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Mar 11, 2020
@jwaisner
Copy link
Member

Closing PR and reopened issue report #16185


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

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