Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-71533

Remove mbstring and htmlpurifier from phpspreadsheet lib

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Minor Minor
    • 3.11
    • 3.11
    • Libraries
    • MOODLE_311_STABLE
    • MOODLE_311_STABLE
    • Hide

      Same as MDL-70314

      Testing scenario 1

      1. Login as admin
      2. Go to http://yourmoodleurl/lib/tests/other/spreadsheettestpage.php and generate the Excel sample file.
      3. Review the server error logs to make sure there are no notices there.
      4. Open the Excel sample file in various office suites.
      5. Expected Result: Check the content is displayed as expected (Font colors, headings etc). You can download the ODS file and compare with it or download the Excel file without that patch. Alternatively, you can also compare to the moodletest_withoutpatch.xlsx file attached to MDL-70314 (which was created without the patch).
        1. NOTE: That you'll notice a change in default font size & font family within the XLSX file and that this is expected 

      Testing scenario 2

      1. Login as admin.
      2. Create a course with some enrolled students and one graded activity for some of them (at least).
      3. Access to the course.
      4. Go to Grades / Export / Excel spreadsheet
      5. Click over the Download button
      6. Expected Result: Check the file is downloaded and contains one row per user and, at least, the following columns:  "First name", "Surname", "ID number", "Institution", "Department", "Email address", "Course total", "Last downloaded from this course" and one column for each activity.
      Show
      Same as MDL-70314 Testing scenario 1 Login as admin Go to  http://yourmoodleurl/lib/tests/other/spreadsheettestpage.php  and generate the Excel sample file. Review the server error logs to make sure there are no notices there. Open the Excel sample file in various office suites. Expected Result: Check the content is displayed as expected (Font colors, headings etc). You can download the ODS file and compare with it or download the Excel file without that patch. Alternatively, you can also compare to the moodletest_withoutpatch.xlsx  file attached to MDL-70314 (which was created without the patch). NOTE: That you'll notice a change in default font size & font family within the XLSX file and that this is expected  Testing scenario 2 Login as admin. Create a course with some enrolled students and one graded activity for some of them (at least). Access to the course. Go to Grades / Export / Excel spreadsheet Click over the Download button Expected Result: Check the file is downloaded and contains one row per user and, at least, the following columns:  "First name", "Surname", "ID number", "Institution", "Department", "Email address", "Course total", "Last downloaded from this course" and one column for each activity.

      The symfony polyfill for mbstring triggers a php lint failure in PHP 7.4

      php7.4 -l lib/phpspreadsheet/vendor/symfony/polyfill-mbstring/bootstrap80.php

      I understand HQ doesn't run a full lint on the codebase and excludes vendor dirs, but we've been doing that for years in our CI and has been passing for a long time (and passes on all supported versions prior to 3.11)

      I see that the phpspreadsheet lib already has other libraries removed as they exist in Moodle, eg:

       /vendor/myclabs/ vendor/maennchen/

      As mbstring has been required since Moodle 3.9, I think the symfony/polyfill-mbstring should be removed as well - which also fixes the lint issue.

      htmlpurifier also already exists in core under lib/htmlpurifier so phpspreadsheet/vendor/ezyyang/htmlpurifier should also be removed.

       

      It would also be interesting to hear if there is a policy for ignoring Lint failures in 3rd party libs included in core?

        1. MDL-71533.jpg
          MDL-71533.jpg
          59 kB
        2. MDL-71533 (2).jpg
          MDL-71533 (2).jpg
          54 kB
        3. moodletest_withoutpatch.xlsx
          12 kB

            danmarsden Dan Marsden
            danmarsden Dan Marsden
            Mathew May Mathew May
            Sara Arjona (@sarjona) Sara Arjona (@sarjona)
            Anna Carissa Sadia Anna Carissa Sadia
            Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved:

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 hour, 50 minutes
                1h 50m

                  Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.