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

podpora pro nette 3.1 a php 8.3 #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

krejzyeu
Copy link
Contributor

@krejzyeu krejzyeu commented Jan 15, 2024

Přidává podporu pro nette 3.1 a php 8.3

  • instalace phpstan/phpstan-nette kvůli odstranění erroru při pracování s $this->template->link
  • template nemá v nette 3.1 již metodu add, proto přepsáno nette způsob, vytváření proměnných

Copy link

@VojtechBuba VojtechBuba left a comment

Choose a reason for hiding this comment

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

Zkus prosím zvážit navrhované úpravy. Takto by to opravdu mohlo jít ven bez BC breaku.

if ($template instanceof Template) {
$template->add('link', new AsyncControlLink($linkMessage, $linkAttributes));
}
$template->link = new AsyncControlLink($linkMessage, $linkAttributes);

Choose a reason for hiding this comment

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

Tady jsou dvě mezery za rovnítkem

Suggested change
$template->link = new AsyncControlLink($linkMessage, $linkAttributes);
$template->link = new AsyncControlLink($linkMessage, $linkAttributes);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Díky za postřeh. Upraveno

composer.json Outdated
@@ -21,13 +21,14 @@
}
],
"require": {
"php": "~7.4 | ~8.0",
"nette/application": "^2.4"
"php": "~7.4 | 8.0 - 8.3",

Choose a reason for hiding this comment

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

Ja bych tady nechal ty původní hodnoty. Tímto říkáme, jakou verzi vyžaduje náš kód v knihovně, neřešil bych podporu ostatních knihoven. Když vyjde php 8.4 tak nejspíš bude náš kód stále kompatibilní. Pokud by závislosti přidaly podporu php 8.4 v minor verzích, tak ani nebudeme muset vydávat novou verzi.

Suggested change
"php": "~7.4 | 8.0 - 8.3",
"php": "~7.4 | ~8.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

composer.json Outdated
"php": "~7.4 | ~8.0",
"nette/application": "^2.4"
"php": "~7.4 | 8.0 - 8.3",
"nette/application": "^3.0"

Choose a reason for hiding this comment

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

Tady je otázka zdali ten přístup skrze magický setter, nebyl podporovaný už v 2.4. Pokud ano, tak by tady klidně mohly být obě verze, protože knihovně to je jedno a jde nám pouze o kompatibilitu s vyšší verzí nette/application při jejím použití.

Suggested change
"nette/application": "^3.0"
"nette/application": "^2.4 | ^3.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dobrý nápad. Magický setter podporovaný v nette samozřejmě je.
Musel jsem upravit test, aby procházel v obou verzích. nette/application od 3.0 rozšiřuje metodu TemplateFactory::createTemplate o další parametr. Aby byl jeden test tak je třeba ověřovat správnost přes callback v Mockery.

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.

2 participants