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

Remove 2 lines of commented code in the updater library #11520

Merged
merged 1 commit into from Aug 8, 2016

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Aug 8, 2016

Summary of Changes

This PR just removes two lines of commented code

Testing Instructions

This just remove 2 comments so i don't see how this can be tested ;)

Documentation Changes Required

None 😄

@jeckodevelopment
Copy link
Member

I have tested this item ✅ successfully on a429f4d

On code review.


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

@grhcj
Copy link

grhcj commented Aug 8, 2016

I have tested this item ✅ successfully on a429f4d

On Review


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

@brianteeman
Copy link
Contributor

Are you sure about this. Has the todo ben done

On 8 August 2016 at 19:59, Lukas Mentel notifications@github.com wrote:

I have tested this item ✅ successfully on a429f4d
a429f4d

On Review

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/11520
https://issues.joomla.org/tracker/joomla-cms/11520.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11520 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8YpEwlBE9dMxxlAoFXFxZ2aZho_Mks5qd3yvgaJpZM4JfUfi
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 8, 2016

@todo remove code ?

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 8, 2016

-       // @todo remove code: if(!isset($this->$tag->_data)) $this->$tag->_data = '';
-       // @todo remove code: $this->$tag->_data .= $data;

@brianteeman
Copy link
Contributor

brianteeman commented Aug 8, 2016 via email

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 8, 2016

The todo was to remove the commented code ;)

    /**
     * Character Parser Function
     *
     * @param   object  $parser  Parser object.
     * @param   object  $data    The data.
     *
     * @return  void
     *
     * @note    This is public because its called externally.
     * @since   11.1
     */
    public function _characterData($parser, $data)
    {
        $tag = $this->_getLastTag();
        // @todo remove code: if(!isset($this->$tag->_data)) $this->$tag->_data = '';
        // @todo remove code: $this->$tag->_data .= $data;
        // Throw the data for this item together
        $tag = strtolower($tag);
        if ($tag == 'tag')
        {
            $this->currentUpdate->stability = $this->stabilityTagToInteger((string) $data);
            return;
        }
        if (isset($this->currentUpdate->$tag))
        {
            $this->currentUpdate->$tag->_data .= $data;
        }
    }

There is no code like that in that method (expected the comment itself)

@brianteeman
Copy link
Contributor

brianteeman commented Aug 8, 2016 via email

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 8, 2016

Yes 😄

@wilsonge wilsonge merged commit ff7e512 into joomla:staging Aug 8, 2016
@wilsonge wilsonge added this to the Joomla 3.6.3 milestone Aug 8, 2016
@wilsonge
Copy link
Contributor

wilsonge commented Aug 8, 2016

Those lines were done in a platform update in 2012. I'm pretty confident we'd have found any issues in the last 4 years if there were any :P

@zero-24 zero-24 deleted the patch-14 branch August 8, 2016 20:11
roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016
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

6 participants