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

Bump dependencies to support Illuminate v11 components #702

Merged
merged 12 commits into from
Mar 26, 2024
Merged

Conversation

damiani
Copy link
Contributor

@damiani damiani commented Mar 19, 2024

This PR upgrades the following key dependencies:

  • Illuminate components to v11
  • PHPUnit to v11

In the process, this required upgrades two Markdown-related dependencies:

The front-yaml upgrade adds a string return type declaration to the parse method; since modifying Jigsaw's markdown parser isn't documented (other than using the commonmark alternative as mentioned in #687, which should be unaffected), I don't think we need to consider this a breaking change.

(This PR builds on #701, from @njames, focusing solely on bumping underlying dependencies to Illuminate v11 components, without the CS fixes and the addition of Laravel Prompts. It supersedes #677.)

@damiani damiani requested a review from bakerkretzmar March 19, 2024 20:07
@damiani
Copy link
Contributor Author

damiani commented Mar 19, 2024

@bakerkretzmar Would love to get your eyes on this, and hear your thoughts about the front-yaml upgrade 😄

@njames
Copy link
Contributor

njames commented Mar 21, 2024

Hey @damiani

I have installed a test blog locally jigsaw init blog and it all looks correct when served

I did get a collections error:

   WARN  Creation of dynamic property TightenCo\Jigsaw\Collection\CollectionItem::$type is deprecated in /Users/nigeljames/OS/jigtest/source/_layouts/post.blade.php on line 3.

and there is one test failing locally

Building local site
Building files from source...


   WARN  Creation of dynamic property TightenCo\Jigsaw\Collection\CollectionItem::$type is deprecated in /Users/nigeljames/OS/jigtest/source/_layouts/post.blade.php on line 3.

I ran ./vendor/bin/php-cs-fixer fix which I have added in the most recent commit and this has not impacted either of the two issues above.

@njames
Copy link
Contributor

njames commented Mar 21, 2024

With regard to the ci suite the composer was bumped to 8.2 for being able to add laravel\prompts but this could go back to 8.1 (unless there was something else that was requiring 8.2 when I went though the process.

Otherwise we could relax this to 8.1 and merge it and then tag a v2 with 8.2 as the minimum version to which we can add new features like prompts.

Happy to help in this process any way I can.

Cheers,
Nigel

@bakerkretzmar
Copy link
Collaborator

@damiani this looks good to me! I think the fact that overriding that method was never documented does make this less of a concern re: backwards compatibility. We might as well drop PHP 8.0 and 8.1 from the test suite here too, and that change I think is fine to release without a major version bump.

@damiani
Copy link
Contributor Author

damiani commented Mar 21, 2024

@njames What is the test you have failing locally? Your output shows a deprecation notice.
Regarding that deprecation notice, would you be willing to post a repo of your jigtest test site so I can take a closer look?

@njames
Copy link
Contributor

njames commented Mar 21, 2024

Thanks @damiani

https://github.com/njames/jigtest

was created from my local version of jigsaw and I used d to delete whatever was there.

@njames
Copy link
Contributor

njames commented Mar 21, 2024

This is more detail from the failed test running locally - ( I am running PHP 8.3.3 from herd on an intel MBP in this instance )

There was 1 error:

1) Tests\SnapshotsTest::build with data set "default" ('default')
Symfony\Component\Process\Exception\ProcessFailedException: The command "'php' '/Users/nigeljames/OS/jigsaw/jigsaw' 'build' '-vvv'" failed.

Exit Code: 1(General error)

Working directory: /Users/nigeljames/OS/jigsaw/tests/snapshots/default

Output:
================


Building local site
Loading collections...
    0 [>---------------------------]
 6/6 [============================] 100%
 10/10 [============================] 100%
 19/19 [============================] 100%
 20/20 [============================] 100%
 27/27 [============================] 100%
 32/32 [============================] 100%
 35/35 [============================] 100%Building files from source...
  0/90 [>---------------------------]   0%
 18/90 [=====>----------------------]  20%
 36/90 [===========>----------------]  40%
   Spatie\LaravelIgnition\Exceptions\ViewException

  syntax error, unexpected token "}"

  at cache/8bc72d715369b34b0e11aa18ee47f329bc145b8e.blade.md:18
     14▕     @endif
     15▕ @endverbatim
     16▕
     17▕ ```
  ➜  18▕ <?{{ '' }}php
     19▕ ```
     20▕
     21▕ <hr>
     22▕ Start...

  1   cache/a303bb2378cbdab46e4b9ac8f63ea68c4b40f3ee.blade.php:3
      Illuminate\View\View::render()

@damiani
Copy link
Contributor Author

damiani commented Mar 21, 2024

Odd, that test passes for me locally and in CI. So I'm thinking we ignore that local failure of yours for now :)
I've handled the deprecation notice, so I think we're good!

@njames
Copy link
Contributor

njames commented Mar 22, 2024

Interestingly on my Linux machine PHP 8.3.4 / valet all the tests pass

@damiani
Copy link
Contributor Author

damiani commented Mar 22, 2024

Aha, same here ... it fails on 8.3.3, passes on 8.3.4. I'll look at this a little closer ... this particular test is for a pretty obscure edge-case, but I'm curious what changed between versions.

@damiani damiani merged commit 2565738 into main Mar 26, 2024
4 of 5 checks passed
@damiani damiani deleted the kdd/illuminate-11 branch March 26, 2024 17:03
@damiani
Copy link
Contributor Author

damiani commented Mar 26, 2024

The test failure on 8.3.3 is for an obscure escaped verson of <?php inside a fenced code block that I imagine literally nobody is using, since we automatically escape this internally now ... so I've gone ahead and updated the test.

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.

3 participants