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

MW 1.24+ / Hotfix for missing Redirect caused by bug 62856 #420

Merged
merged 1 commit into from Jul 18, 2014

Conversation

mwjames
Copy link
Contributor

@mwjames mwjames commented Jul 18, 2014

MW-core has yet to provide a working solution (which it hasn't) but to avoid further delay (due the ContentHandler on redirects #212 ) and to fix the problem forceToUseParser is added to avoid the ContentHandler usage during the Update.

SimplePageRedirectRegressionTest is running again on MW 1.24+

@mwjames mwjames added this to the SMW 2.0 milestone Jul 18, 2014
@mwjames mwjames mentioned this pull request Jul 18, 2014
24 tasks
* @bug 62856 and #212
* @since 1.9
*/
public function forceToUseParser() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see you really like fluent interfaces :)

This is misplaced here though IMO. If this where a builder, then sure. But parse does not return an instance of a new build thing. It's a method with side effects.

Did you see something like this somewhere else, or recommended somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But parse does not return an instance of a new build thing

It is just a switch, so I can do $contentparser->forceToUseParser()->parse() without having to do:

$contentparser->forceToUseParser();
$contentparser->parse();

If it weren't for the bug I wouldn't need this but since the ContentHandler sucks ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two liner is better. With the one liner, I find it odd that forceToUseParser changes the state of the $contentparser. It's hiding the side effect.

the ContentHandler sucks

Perhaps, though that is no reason to abuse fluent interfaces here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two liner is better.
Perhaps, though that is no reason to abuse fluent interfaces here.

If this makes you happy so be it .. I'm low on battery to argue for or against it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

MW-core has yet to provide a working solution (which it hasn't)  but to avoid further delay
(due the ContentHandler on redirects) and to fix the problem `forceToUseParser`
is added to avoid the `ContentHandler` usage during the Update.

SimplePageRedirectRegressionTest is running again on MW 1.24+
@scrutinizer-notifier
Copy link

A new inspection was created.

JeroenDeDauw added a commit that referenced this pull request Jul 18, 2014
MW 1.24+ / Hotfix for missing Redirect caused by bug 62856
@JeroenDeDauw JeroenDeDauw merged commit 87c8436 into master Jul 18, 2014
@JeroenDeDauw JeroenDeDauw deleted the bug-62856 branch July 18, 2014 16:30
@mwjames
Copy link
Contributor Author

mwjames commented Jul 18, 2014

👍 now master only omits 7 tests which all require SPARQLStore.

@JeroenDeDauw
Copy link
Member

\o/

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

Successfully merging this pull request may close these issues.

None yet

3 participants