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

Already on GitHub? Sign in to your account

Resolving possible Undefined variable notice $currentAssetId #1765

Merged
merged 3 commits into from Dec 20, 2012

Conversation

Projects
None yet
5 participants
Contributor

betweenbrain commented Dec 18, 2012

Undefined variable: currentAssetId in /libraries/joomla/table/table.php on line 778 may occur, please see https://github.com/JTracker/jissues/issues/44 for the related issue for discovery

Contributor

elinw commented Dec 19, 2012

Can't you have an empty $this->assetid when there is no $currentAssetId? In fact won't that always happen? https://github.com/betweenbrain/joomla-platform/blob/a1d8d1a1db9e09f6d6de48930af758213ee127b1/libraries/joomla/table/table.php#L698

Maybe better either to actually define$currentAssetId for the other cases or to add a check for it being set to the second condition.

Owner

mbabker commented Dec 19, 2012

$currentAssetId isn't always set though, and that's the issue he's trying to resolve here.

Contributor

pasamio commented Dec 19, 2012

Why not on line 697 just default it to zero to ensure it is always set and if there is an asset ID in the object then it will be updated but it is always defined. Perhaps it is a habit I have left over from languages where if you define something in a block that it isn't available externally but I prefer this and it is only an extra line to add. Perhaps I am missing a nuance here but would that solve the problem as well?

Contributor

elinw commented Dec 19, 2012

Exactly, setting it for every case makes more sense then eliminating the possibility of having the first condition of the block he's modifying never be met which defeats half the point of the code in the block. Or as I said the other possibility is to add isset to the second condition. It's most likely less confusing for codebrowsers to set it for everyone though.

@elinw elinw commented on an outdated diff Dec 19, 2012

libraries/joomla/table/table.php
- // Create an asset_id or heal one that is corrupted.
- if (empty($this->asset_id) || ($currentAssetId != $this->asset_id && !empty($this->asset_id)))
- {
- // Update the asset_id field in this table.
- $this->asset_id = (int) $asset->id;
-
- $query = $this->_db->getQuery(true);
- $query->update($this->_db->quoteName($this->_tbl));
- $query->set('asset_id = ' . (int) $this->asset_id);
- $this->appendPrimaryKeys($query);
- $this->_db->setQuery($query);
-
- $this->_db->execute();
+ if (isset($currentAssetId)) {
+ // Create an asset_id or heal one that is corrupted.
+ if (empty($this->asset_id) || ($currentAssetId != $this->asset_id && !empty($this->asset_id))) {
@elinw

elinw Dec 19, 2012

Contributor

if (empty($this->asset_id) || (isset($currentAssetId) && $currentAssetId != $this->asset_id && !empty($this->asset_id)))

would be one option here.

or you could add
else
{
$currentAssetId = 0
}

where $currentAssetId is set for the cases where $this->asset_id is !empty. Or just put it before and save 3 lines of code.

Contributor

betweenbrain commented Dec 19, 2012

Michael is exactly right in that the issue I'm trying to solve is that $currentAssetId isn't always set with the existing code. My approach was to change the existing code as little as possible. That said, Sam's solution of setting it to 0 on line 697 is a great way to approach it and would actually simplify line 779. If that's the approach the maintainers would prefer, I'd be happy to update my PR.

Owner

mbabker commented Dec 19, 2012

Looks to be a better solution. Now we just need to get the codestyle fixed, and I think we're good to go. http://developer.joomla.org/pulls/pulls/1765.html

Contributor

elinw commented Dec 19, 2012

Yes I believe that is exactly what I said in my first reply. It's always set when the first condition is not true but it makes sense to do it because who knows what could happen in the intervening lines.

Contributor

betweenbrain commented Dec 19, 2012

Thanks Michael. I'll work on tweaking my IDE settings to get the code style
correct.

Best,

Matt

Sent from my phone that uses an open source operating system.
On Dec 19, 2012 2:24 PM, "Michael Babker" notifications@github.com wrote:

Looks to be a better solution. Now we just need to get the codestyle
fixed, and I think we're good to go.
http://developer.joomla.org/pulls/pulls/1765.html


Reply to this email directly or view it on GitHubhttps://github.com/joomla/joomla-platform/pull/1765#issuecomment-11544090.

ianmacl added a commit that referenced this pull request Dec 20, 2012

Merge pull request #1765 from betweenbrain/currentAssetId
Resolving possible Undefined variable notice $currentAssetId

@ianmacl ianmacl merged commit 19480ec into joomla:staging Dec 20, 2012

Contributor

betweenbrain commented Dec 20, 2012

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment