Skip to content

Commit

Permalink
Item13897: Fixes of problems.
Browse files Browse the repository at this point in the history
- Added 'last resort' try/catch in Engine::CGI to display lost exceptions.
- Moo-fied Foswiki::If::Parser, Foswiki::Query::Parser.
- Changed 'isa' to 'is' in some 'has' clauses (no good of typing faster
    than thinking :) ).
  • Loading branch information
vrurg committed Jan 6, 2016
1 parent bcec3f0 commit cd395e0
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 50 deletions.
20 changes: 18 additions & 2 deletions core/lib/Foswiki/Engine/CGI.pm
Expand Up @@ -16,6 +16,8 @@ use strict;
use warnings;

use CGI;
use CGI::Carp;
use Data::Dumper;

This comment has been minimized.

Copy link
@MichaelDaum

MichaelDaum Jan 6, 2016

Member

Is Data::Dumper really used anywhere?

This comment has been minimized.

Copy link
@vrurg

vrurg Jan 6, 2016

Author Member

Michael, it's a leftover of an experiment. I'll remove it. As it's an experimental branch glitches like this would sneak in sometimes.

use Foswiki::Engine ();
our @ISA = ('Foswiki::Engine');

Expand All @@ -25,6 +27,7 @@ use Foswiki::Request ();
use Foswiki::Request::Upload ();
use Foswiki::Response ();
use Unicode::Normalize;
use Try::Tiny;

This comment has been minimized.

Copy link
@cdot

cdot Jan 6, 2016

Contributor

As I read it, "ImproveOOModel" is still "UnderInvestigation" and no checkins to master should be done yet (until it shifts state to "Accepted". Aside from anything else, I haven't had a chance to comment properly yet. There has been no discussion about adding Try::Tiny to the dependencies - if we do, we have duplicate exception handling modules.

This comment has been minimized.

Copy link
@jomo666

jomo666 Jan 6, 2016

Afaik vrurg's checkins are into his branch (Item13897), not into the master. Isn't?

This comment has been minimized.

Copy link
@Jlevens

Jlevens Jan 6, 2016

Contributor

If we choose to adopt perl v5.14.2 as the minimum with FW 3.0 (which ImproveOOModel is targeted to) then because of http://perldoc.perl.org/perl5140delta.html#Exception-Handling we may not need any exception handling modules.

(See http://foswiki.org/Development/RequirePerl510From2017x03)

Try::Tiny has a feature that if you 'return' from inside a block you just leave the block. You do not 'return' to the calling sub - a trap for the unwary.

Other exception handling modules are sophisticated enough to eliminate that, but have many dependencies.

The downside is that we lose syntactic sugar and just use standard perl i.e.
eval { ... }
if( $@ ... ) { ... }
elseif( $@... ) { ... }
else {}

Is that so bad?

This comment has been minimized.

Copy link
@vrurg

vrurg Jan 6, 2016

Author Member

cdot – there will be no duplication as Error has to go.

Jlevens – pure eval-based exception handling has worse readability. Don't like it very much. As to the return – yes, it complicates life a bit. But this is a feature of eval. return gets you out of the enclosing eval {} scope.


BEGIN {
if ( $Foswiki::cfg{UseLocale} ) {
Expand Down Expand Up @@ -97,8 +100,21 @@ sub run {
exit 1;
}
if ( UNIVERSAL::isa( $req, 'Foswiki::Request' ) ) {
my $res = Foswiki::UI::handleRequest($req);
$this->finalize( $res, $req );
try {
my $res = Foswiki::UI::handleRequest($req);
$this->finalize( $res, $req );
}
catch {
# Whatever error we get here – we generate valid HTTP response. At least we try...
# This is the last frontier of error handling.
# SMELL XXX Test code.

# untie of the handles is required when remote debugging is used.
untie(*STDOUT) if tied(*STDOUT);
untie(*STDERR) if tied(*STDERR);

CGI::Carp::confess( $_->{-text} );
};
}
}

Expand Down
20 changes: 9 additions & 11 deletions core/lib/Foswiki/If/Parser.pm
Expand Up @@ -10,12 +10,6 @@ Support for the conditions in %IF{} statements.

package Foswiki::If::Parser;

use strict;
use warnings;

This comment has been minimized.

Copy link
@MichaelDaum

MichaelDaum Jan 6, 2016

Member

Why remove strict mode and warnings?

This comment has been minimized.

Copy link
@Jlevens

Jlevens Jan 6, 2016

Contributor

Moo enforces strict and warnings iirc

use Foswiki::Query::Parser ();
our @ISA = ('Foswiki::Query::Parser');

use Assert;
use Foswiki::If::Node ();

Expand All @@ -28,6 +22,11 @@ use Foswiki::If::OP_isempty ();
use Foswiki::If::OP_istopic ();
use Foswiki::If::OP_isweb ();

use Moo;

This comment has been minimized.

Copy link
@cdot

cdot Jan 6, 2016

Contributor

As I read it, "ImproveOOModel" is still "UnderInvestigation" and no checkins to master should be done yet (until it shifts state to "Accepted". The implications of adding a major dependency like Moo needs to be thought out carefully.

This comment has been minimized.

Copy link
@vrurg

vrurg Jan 6, 2016

Author Member

I'm not even sure if this branch would ever make it into the master as-is even if the whole discussion on the topic would settle down The Plan. But even in this case it would serve as a good base and a proof of concept.

use namespace::clean;

extends 'Foswiki::Query::Parser';

BEGIN {
if ( $Foswiki::cfg{UseLocale} ) {
require locale;
Expand All @@ -40,16 +39,15 @@ BEGIN {
use constant OPS => qw(allows context defined dollar ingroup isempty
istopic isweb );

sub new {
my ($class) = @_;
sub BUILD {
my $this = shift;

$this->nodeClass('Foswiki::If::Node');

my $this = $class->SUPER::new( { nodeClass => 'Foswiki::If::Node', } );
foreach my $op ( OPS() ) {
my $on = 'Foswiki::If::OP_' . $op;
$this->addOperator( $on->new() );
}

return $this;
}

1;
Expand Down
4 changes: 2 additions & 2 deletions core/lib/Foswiki/Infix/Error.pm
Expand Up @@ -15,11 +15,11 @@ use namespace::clean;
extends 'Foswiki::Exception';

has expr => (
is => 'ro',
is => 'rw',
predicate => 1,
);
has at => (
is => 'ro',
is => 'rw',
predicate => 1,
);
our @_newParameters = qw(text expr at);
Expand Down
4 changes: 2 additions & 2 deletions core/lib/Foswiki/Infix/Node.pm
Expand Up @@ -37,9 +37,9 @@ use constant {
META => 5,
};

has op => ( isa => 'rw', );
has op => ( is => 'rw', );

has params => ( isa => 'rw', );
has params => ( is => 'rw', );

=begin TML
Expand Down
49 changes: 25 additions & 24 deletions core/lib/Foswiki/Infix/Parser.pm
Expand Up @@ -18,24 +18,22 @@ Escapes are supported in strings, using backslash.

package Foswiki::Infix::Parser;

use strict;
use warnings;
BEGIN {
if ( $Foswiki::cfg{UseLocale} ) {
require locale;
import locale();
}
}

use Assert;
use Try::Tiny;
use Foswiki::Infix::Error;
use Foswiki::Infix::Node ();
use Foswiki::Infix::Node;
use Moo;
use namespace::clean;

extends 'Foswiki::Object';

BEGIN {
if ( $Foswiki::cfg{UseLocale} ) {
require locale;
import locale();
}
}

# Set to 1 for debug
use constant MONITOR_PARSER => 0;

Expand Down Expand Up @@ -84,28 +82,28 @@ be escaped using backslash (\).

# Object properties
has node_factory => (
isa => 'ro',
is => 'ro',
init_arg => 'nodeClass',
);

has operators => (
isa => 'rw',
default => sub { []; },
is => 'rw',
default => sub { return []; },
);

has initialised => (
isa => 'rw',
is => 'rw',
default => 0,
);

has numbers => (
isa => 'rw',
default => qr/(\d+\.\d+|\d+\.|\.\d+|\d+)([eE][+-]?\d+)?/,
is => 'rw',
default => sub { qr/(\d+\.\d+|\d+\.|\.\d+|\d+)([eE][+-]?\d+)?/ },
);

has words => (
isa => 'rw',
default => qr/\w+/,
is => 'rw',
default => sub { qr/\w+/ },
);

# Break circular references.
Expand Down Expand Up @@ -314,15 +312,18 @@ s/(?<!\\)\\(0[0-7]{2}|x[a-fA-F0-9]{2}|x\{[a-fA-F0-9]+\}|n|t|\\|$q)/eval('"\\'.$1
}
elsif {
# XXX $_ has to be carefully examined
Foswiki::Infix::Error->throw( $_->text, $expr, $$input );
my $text;
if ( $_->isa('Error') || $_->isa('Error::Simple') ) {
$text = $_->{-text};
}
else {
$text = $_->text;
}

Foswiki::Infix::Error->throw( $text, $expr, $$input );
}
};

#catch Error with {

# # Catch errors thrown during the tree building process
# throw Foswiki::Infix::Error( shift, $expr, $$input );
#};
Foswiki::Infix::Error->throw( 'Missing operator', $expr, $$input )
unless scalar(@opands) == 1;
Foswiki::Infix::Error->throw(
Expand Down
25 changes: 16 additions & 9 deletions core/lib/Foswiki/Query/Parser.pm
Expand Up @@ -25,9 +25,6 @@ BEGIN {
}
}

use Foswiki::Infix::Parser ();
our @ISA = ('Foswiki::Infix::Parser');

use Foswiki::Query::Node ();

# operator name precedence
Expand Down Expand Up @@ -69,6 +66,20 @@ use Foswiki::Query::OP_int (); # 1000

use Foswiki::Query::OP_ob (); # 1100

use Moo;
use namespace::clean;

extends 'Foswiki::Infix::Parser';

has words => (
is => 'ro',
default => sub { qr/([A-Z:][A-Z0-9_:]*|({[A-Z][A-Z0-9_]*})+)/i },
);
has nodeClass => (
is => 'rw',
default => 'Foswiki::Query::Node',
);

=begin TML
Query Language BNF
<verbatim>
Expand Down Expand Up @@ -101,17 +112,13 @@ use constant OPS => qw (match and eq lc lte not ref d2n gte length lt ob
uc dot gt like ne or where comma plus minus
neg times div in int );

sub new {
my ( $class, $options ) = @_;
sub BUILD {
my $this = shift;

$options->{words} ||= qr/([A-Z:][A-Z0-9_:]*|({[A-Z][A-Z0-9_]*})+)/i;
$options->{nodeClass} ||= 'Foswiki::Query::Node';
my $this = $class->SUPER::new($options);
foreach my $op ( OPS() ) {
my $on = 'Foswiki::Query::OP_' . $op;
$this->addOperator( $on->new() );
}
return $this;
}

# Ensure there is at least one operand on the opstack when closing
Expand Down

0 comments on commit cd395e0

Please sign in to comment.