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

Switch to cakephp/twig-view 1.0 #653

Merged
merged 1 commit into from
Apr 18, 2020
Merged

Switch to cakephp/twig-view 1.0 #653

merged 1 commit into from
Apr 18, 2020

Conversation

othercorey
Copy link
Member

No description provided.

@othercorey othercorey added this to the 2.x (CakePHP 4) milestone Mar 8, 2020
@ADmad
Copy link
Member

ADmad commented Mar 8, 2020

After switch to the new twigview plugin do we release a new minor for bake or new major?

@dereuromark
Copy link
Member

A minor should be fine, as it will auto pull the needed dependency.

@othercorey
Copy link
Member Author

The only problem with a minor now is we won't be 100% backwards compatible with cakephp/twig-view. The namespaces will change. I don't think there's anything else we're going to change that will affect end users.

@dereuromark
Copy link
Member

For a require dev dependency thats probably okish.

@othercorey
Copy link
Member Author

Can we marked this as replacing wyrihaximus/twig-view in packagist? Will that throw an error for projects that don't update their own require dependency if they use TwigView outside of bake?

@dereuromark
Copy link
Member

If you use conflict you can do that. It can then only install once you remove the other twig dependency.

@ADmad
Copy link
Member

ADmad commented Mar 23, 2020

I think we should use conflict, otherwise there will be problems for those who use twigview directly.

@dereuromark
Copy link
Member

dereuromark commented Mar 29, 2020

Now that we have a TwigView beta, we should switch to the cake one
It otherwise blocks other plugins from switching

- cakephp/bake dev-master requires wyrihaximus/twig-view ^5.0.1 -> satisfiable by wyrihaximus/twig-view[5.0.1].
- cakephp/bake dev-master requires wyrihaximus/twig-view ^5.0.1 -> satisfiable by wyrihaximus/twig-view[5.0.1].
- cakephp/twig-view 1.0.0-beta1 conflicts with wyrihaximus/twig-view[5.0.1].
- cakephp/twig-view 1.0.0-beta1 conflicts with wyrihaximus/twig-view[5.0.1].
- dereuromark/cakephp-dto dev-cake-twig requires cakephp/twig-view ^1.0.0 -> satisfiable by cakephp/twig-view[1.0.0-beta1].
- Installation request for dereuromark/cakephp-dto dev-cake-twig as 1.0.0 -> satisfiable by dereuromark/cakephp-dto[dev-cake-twig].
- Installation request for cakephp/bake dev-master as 2.0.5 -> satisfiable by cakephp/bake[dev-master].

@dereuromark dereuromark marked this pull request as ready for review March 29, 2020 15:16
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@dereuromark
Copy link
Member

The conflict should not go here, though. Right?
It should go to twig-view repo as there we need to make composer aware it cannot be used together with the legacy one.

@ADmad
Copy link
Member

ADmad commented Mar 29, 2020

The conflict should not go here, though. Right?

Yes, it has been added to cakephp/twig-view.

@othercorey
Copy link
Member Author

If we switch to the beta, it requires everyone to move their minimum stability again. Not sure we want to do that.

@dereuromark
Copy link
Member

dereuromark commented Mar 29, 2020

Then release a stable plugin version now.
At this point it is as stable as the other one before - and we can add patches for things that are broken.

Without having a stable release this is blocking now other plugins to stabilize anyway.

@othercorey
Copy link
Member Author

Can no one try it in a project somewhere once?

@dereuromark
Copy link
Member

I already did as part of Dto plugin (see link), and the sandbox using it from project level.
But I didn't use it as project only templating. Maybe someone else can
Or you can add a demo of it to the sandbox.

@ADmad
Copy link
Member

ADmad commented Apr 7, 2020

I reverted back to use dev-master of TwigView. Given the flurry of changes we are making to TwigView it's better to keep testing against bake regularly, instead of waiting till next beta to discover bugs/BC breaks.

@ADmad
Copy link
Member

ADmad commented Apr 7, 2020

As I had suspected there were issues which I have fixed.

@othercorey
Copy link
Member Author

There are missing variable exceptions in the unit tests with strict_variables => true.

@dereuromark
Copy link
Member

prefix + traitName
not too bad

@othercorey
Copy link
Member Author

I could not fix this error:

  1. Bake\Test\TestCase\Command\TemplateCommandTest::testGetContentWithRoutingPrefix
    Twig\Error\RuntimeError: Impossible to access an attribute ("type") on a null variable.

/cake/templates/bake/element/form.twig:52

The test is faking data that always loads the $modelObject. However, setting modelObject to the model instance causes another failure. I'm not sure if is valid.

@othercorey othercorey force-pushed the twig-view-1.0 branch 2 times, most recently from 30c08fb to 1efdc00 Compare April 11, 2020 22:37
@ewbarnard
Copy link
Contributor

File docs/en/development.rst, section "Bake Template Syntax" links to twig 2.x, saying that Bake uses the Twig 2.x template syntax. cakephp/twig-view uses current Twig (3.x), correct? Given that we link to current Twig at the top of development.rst we could probably revert the link to plain text "Bake template files use the Twig template syntax."

@othercorey othercorey changed the title Switch to cakephp/twig-view dev-master Switch to cakephp/twig-view 1.0 Apr 17, 2020
@othercorey
Copy link
Member Author

Updated to official 1.0 release.

@othercorey
Copy link
Member Author

  • Added Bake.twigStrictVariables config that's set by unit tests
  • Updated controller.twig to generates @property/@method tags that pass phpcs spacing.

@othercorey othercorey merged commit 5174230 into master Apr 18, 2020
@othercorey othercorey deleted the twig-view-1.0 branch April 18, 2020 00:06
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