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

Joomla 4.0: Removing deprecated code from JViewLegacy #12263

Merged
merged 7 commits into from Oct 4, 2016

Conversation

Projects
None yet
4 participants
@Hackwar
Member

Hackwar commented Oct 2, 2016

Regardless of what we do with JView, we should finally remove the deprecated code from JViewLegacy, that has been sitting there deprecated since 1.6...

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 2, 2016

I've decided to move the JView* classes from /libraries/legacy to /libraries/cms, too.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 2, 2016

I have no idea why the JApplicationWeb tests now fail with this latest change. (moving the tests into the other folder.)

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 2, 2016

My guess is something in the moved tests that changes global state and doesn't reset things. A far too common problem with our test suite.

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 2, 2016

https://github.com/Hackwar/joomla-cms/blob/4d629f86c8715fd9760cbdb48cd9147f0f0ccb11/tests/unit/suites/libraries/cms/view/JViewLegacyTest.php#L429-L431

Probably those lines. Instead of restoring the state of $_SERVER after the test it's just setting the values if not previously set.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 2, 2016

Funny, the issue is JPATH_COMPONENT. If it is defined, it fails, if it is not, the JApplicationWeb works fine. Of course JViewLegacy then fails spectacularly... I have no idea what is depending on JPATH_COMPONENT not being defined.

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 2, 2016

JViewLegacy only needs JPATH_COMPONENT if it's instantiated without a base_path parameter in the config array. So change the tests to inject that and you can get around needing the constant?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 2, 2016

Unfortunately doesn't work. It seems as if it is not the issue of the JPATH_COMPONENT constant, but something that is executed in the tests afterwards. I simply commented out the define() for JPATH_COMPONENT and that threw a bunch of errors and actually stopped the JViewLegacy tests. Now I've rewritten those and don't get the JPATH_COMPONENT errors, but get the other 2 failures again.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 2, 2016

Found the issue. JUri::base() throws this of...

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 2, 2016

Fixed it. JUri::reset() in tearDown() fixes this.

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 3, 2016

For those unit tests fixes can we put them into staging? Because that's just good practice anyhow. I'm happy to merge a PR with just those changes in on review

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 3, 2016

The unittests in 3.7 are somehow funky, so to be honest, I don't really want to mess around there...

@wilsonge wilsonge added this to the Joomla 4.0 milestone Oct 3, 2016

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 3, 2016

It's fine. I just directly cherry-picked it into staging :p 0724b0d

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 3, 2016

I've also done a PR for the newsfeed change to staging #12296

I'm trying to keep things in sync as best I can for my sanity in the future when i'm solving conflicts xD

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 3, 2016

Then you shoulda started from the 3.7 branch. My office will have a drink
in memory of your sanity when you have to merge that.

On Monday, October 3, 2016, George Wilson notifications@github.com wrote:

I've also done a PR for the newsfeed change to staging #12296
#12296

I'm trying to keep things in sync as best I can for my sanity in the
future when i'm solving conflicts xD


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

@wilsonge wilsonge merged commit fda5d13 into joomla:4.0-dev Oct 4, 2016

2 checks passed

continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment