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

Remove mbstring and htmlpurifier from phpspreadsheet lib

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.11
    • Fix Version/s: 3.11
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      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.
    • Affected Branches:
      MOODLE_311_STABLE
    • Fixed Branches:
      MOODLE_311_STABLE
    • Pull 3.11 Branch:
      m311_MDL-71533
    • Pull Master Branch:

      Description

      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?

        Attachments

        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

          Issue Links

            Activity

              People

              Assignee:
              danmarsden Dan Marsden
              Reporter:
              danmarsden Dan Marsden
              Peer reviewer:
              Mathew May Mathew May
              Integrator:
              Sara Arjona (@sarjona) Sara Arjona (@sarjona)
              Tester:
              Anna Carissa Sadia Anna Carissa Sadia
              Participants:
              Component watchers:
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona)
              Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Fix Release Date:
                17/May/21

                  Time Tracking

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