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 be_strict. #968

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Conversation

drgrice1
Copy link
Member

All modules should call use strict and certainly have no need of this method. They are not restricted from using use as they are outside of the safe zone. They should also all call use warnings. More work is needed because many of the modules actually have problems that cause warnings.

Macros can (and should) call BEGIN { strict->import }. This is actually entirely equivalent to previously calling BEGIN { be_strict() } (due to an incorrect import call -- see below) and can be done in the safe compartment. Note that use strict is (essentially) the equivalent of BEGIN { require strict; strict->import; }. For macros there is no need to require strict as strict will already have been loaded. So using BEGIN { strict->import } does essentially what use strict does for a macro.

This does not go as far as is really needed. All macros should call BEGIN { strict->import; }. This should be a requirement for macros in PG going forward. This is added in parserGraphTool.pl in this pull request, and the one issue with that macro is fixed. This is deferred for other macros.

Also remove the unused ww_strict.pm module. Note that module was not actually used even by the be_strict method. The be_strict method was defined by

sub be_strict {
	require ww_strict;
	strict::import();
	return;
}

This means that the ww_strict module was required, but the line strict::import() did not import that module. Instead that imports the real perl strict module. In order to import the ww_strict module it would have needed to call ww_strict->import. Notice that the be_strict method also used an incorrect way of calling the import method of strict. The import method is a package method that should be called with an invocant (via strict->import, and not a static method which would be called as strict::import() as was done in the be_strict method. It worked though because the import method doesn't use the invocant, and the be_strict method did not accept any arguments.

@drgrice1 drgrice1 force-pushed the be-strict-removal branch 3 times, most recently from 45e8868 to cde44d1 Compare December 5, 2023 16:27
Copy link
Member

@drdrew42 drdrew42 left a comment

Choose a reason for hiding this comment

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

I looked through a few problems to test this out -- things look good 👍

Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

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

This looks good. Tested a number of problems and work as expected. Requiring use strict (and use warnings) for modules as well as strict->import; for macros will be good moving forward.

All modules should call `use strict` and certainly have no need of this
method.  They are not restricted from using `use` as they are outside of
the safe zone.  They should also all call `use warnings`.  More work is
needed because many of the modules actually have problems that cause
warnings.

Macros can (and should) call `BEGIN { strict->import }`. This is
actually entirely equivalent to previously calling `BEGIN { be_strict() }`
(due to an incorrect `import` call -- see below) and can be done in the
safe compartment. Note that `use strict` is (essentially) the equivalent
of `BEGIN { require strict; strict->import; }`.  For macros there is no
need to `require strict` as `strict` will already have been loaded. So
using `BEGIN { strict->import }` does essentially what `use strict` does
for a macro.

This does not go as far as is really needed.  All macros should call
`BEGIN { strict->import; }`.  This should be a requirement for macros in
PG going forward.  This is added in `parserGraphTool.pl` in this pull
request, and the one issue with that macro is fixed.  This is also added
in `parserRadioMultiAnswer.pl` which has no issues with it. This is
deferred for other macros.

Also remove the unused `ww_strict.pm` module.  Note that module was not
actually used even by the `be_strict` method.  The `be_strict` method
was defined by

```perl
sub be_strict {
	require ww_strict;
	strict::import();
	return;
}

```

This means that the `ww_strict` module was required, but the line
`strict::import()` did not import that module.  Instead that imports the
real perl `strict` module. In order to import the `ww_strict` module it
would have needed to call `ww_strict->import`. Notice that the
`be_strict` method also used an incorrect way of calling the `import`
method of `strict`. The `import` method is a package method that should
be called with an invocant (via `strict->import`, and not a static
method which would be called as `strict::import()` as was done in the
`be_strict` method.  It worked though because the `import` method
doesn't use the invocant, and the `be_strict` method did not accept any
arguments.
@pstaabp pstaabp merged commit da7dc73 into openwebwork:develop Jan 24, 2024
2 checks passed
@drgrice1 drgrice1 deleted the be-strict-removal branch January 24, 2024 22:40
@Alex-Jordan
Copy link
Contributor

Today on develop, I get lots of perl warnings from problems. It seems related to this, which was merged yesterday. I get warnings from a problem like:

WeBWorK Warnings

WeBWorK has encountered warnings while processing your request. If this occurred when viewing a problem, it was likely caused by an error or ambiguity in that problem. Otherwise, it may indicate a problem with the WeBWorK system itself. If you are a student, report these warnings to your professor to have them corrected. If you are a professor, please consult the warning output below for more information.
Warning messages

    Non fatal warnings. These are only displayed for users with permission to view problem debugging info.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/core/PGstandard.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/core/PGbasicmacros.pl.
    Unquoted string "strict" may clash with future reserved word at line 32 of [PG]/macros/core/PGbasicmacros.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/core/PGanswermacros.pl.
    Unquoted string "strict" may clash with future reserved word at line 132 of [PG]/macros/core/PGanswermacros.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/math/PGnumericevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 63 of [PG]/macros/math/PGnumericevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/answers/PGfunctionevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 53 of [PG]/macros/answers/PGfunctionevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/answers/PGstringevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 38 of [PG]/macros/answers/PGstringevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/answers/PGmiscevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 32 of [PG]/macros/answers/PGmiscevaluators.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/core/PGauxiliaryFunctions.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/core/PGcommonFunctions.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/math/customizeLaTeX.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/misc/PGunion.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/misc/unionMacros.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/misc/unionUtils.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/misc/unionProblem.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/graph/unionImage.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/ui/unionLists.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/ui/unionTables.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/misc/unionMessages.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [TMPL]/setOrientation/parserOrientation.pl.
    Unquoted string "strict" may clash with future reserved word at line 1 of [PG]/macros/PGcourse.pl.

@Alex-Jordan
Copy link
Contributor

It seems to come from the BEGIN { strict->import; } in the macro files.

@drgrice1
Copy link
Member Author

I assume that you have restarted the webwork2 app, right?

Is this happening on the problem page?

What version of perl are you using?

@Alex-Jordan
Copy link
Contributor

I have restarted the app. I see this on the problem's page as well as the editor page.

This is on an Oracle 8.7 server, with perl 5.26.3. My production server is the same OS, but perl is 5.36.1. Seems like the perl version is the issue. If it's for sure that we don't want to support this old of perl, I will try to upgrade perl on my dev server.

@drgrice1
Copy link
Member Author

It seems that perl 5.34 or newer is needed for this. I tested with versions 5.26, 5.28, 5.30, and 5.32, and all of those have this issue.

I have a solution that will make this work for those versions though. The strict module just needs to be shared to the safe zone by adding it to the $pg{modules} list in defaults.conf for webwork2.

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.

4 participants