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

add sylius version to the footer in admin #8680

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

gabiudrescu
Copy link
Contributor

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
Related tickets fixes #8679
License MIT

It seems the version was missing from admin due to a bug in a twig file - the footer block in admin got renamed post_content at some point in history.

On top of fixing this bug, I added a new constant in the kernel class, APP_NAME with the default value being "Sylius". In case at a later point someone decides to craft a forked version of Sylius called "Supercharged Sylius" and it needs to brag about the name. Before hand, Sylius was hardcoded in the twig template.

I have declared both Sylius\Bundle\CoreBundle\Application\Kernel::VERSION and Sylius\Bundle\CoreBundle\Application\Kernel::APP_NAME as twig global variables and used them to be in the twig layout template for admin.

I consider this useful when debugging a Sylius application over the phone with a non-technical person. First question I would ask is: what version of Sylius do you run?

Also, I find it a good reason to brag about: look, I'm running Sylius 1.0.0!

@Zales0123 Zales0123 added BC Break PRs introducing BC breaks (do not even try to merge). Bug Fix labels Sep 18, 2017
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

I'm not sure about this new const APP_NAME. I think we can just hardcode Sylius because a link will open a Sylius website anyway. A developer will need to override block one way or another.

It's also a quasi bc-break because footer block is not used anyway.

@gabiudrescu
Copy link
Contributor Author

let's take them one by one:

  1. APP_NAME I'm ok with that, we can remove it if you consider it too much.
  2. I don't think my PR introduces a BC Break, because the footer block is not defined in Sylius/src/Sylius/Bundle/UiBundle/Resources/views/Layout/sidebar.html.twig nor Sylius/src/Sylius/Bundle/AdminBundle/Resources/views/layout.html.twig; all I did is change Sylius/src/Sylius/Bundle/AdminBundle/Resources/views/layout.html.twig to match the block name in Sylius/src/Sylius/Bundle/UiBundle/Resources/views/Layout/sidebar.html.twig; doing it the other way around would probably mean a BC Break.

@lchrusciel
Copy link
Member

I totally agree with the second point. I just don't think that we need this APP_NAME constant. If you would like to remove it, I'm ready to merge it right away. But if you think, that it would be better to introduce it, then just wait for others and if any of them would merge this PR, I'm also ok with that.

Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

I think that if we can avoid additional constant without any problems - let's avoid it ;) I also don't see a really big benefit of introducing it. Nevertheless, nice work ;)

@gabiudrescu
Copy link
Contributor Author

gabiudrescu commented Sep 19, 2017

OK, I have reverted that change introducing the constant.

@pamil pamil added this to the 1.1 milestone Sep 20, 2017
@pamil
Copy link
Contributor

pamil commented Oct 10, 2017

Couldn't spot any BC break here, is it still there? /cc @Zales0123

@lchrusciel
Copy link
Member

For me it is good to go

@@ -61,3 +61,8 @@ fos_rest:
rules:
- { path: '^/api/.*', priorities: ['json', 'xml'], fallback_format: json, prefer_extension: true }
- { path: '^/', stop: true }

twig:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add it to @CoreBundle/Resources/config/app/config.yml instead? Btw. we can name it sylius_meta.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@lchrusciel lchrusciel merged commit 9cae3f2 into Sylius:master Oct 16, 2017
@lchrusciel
Copy link
Member

Thanks Gabi!

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
add sylius version to the footer in admin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break PRs introducing BC breaks (do not even try to merge).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding the Sylius version in admin
5 participants