? ? Pending

User tests: Successful: Unsuccessful:

avatar nikosdion
nikosdion
19 Oct 2019

Pull Request for Issue #26583 .

This PR is no longer valid. Please test the new PR, gh-26744 instead.

avatar nikosdion nikosdion - open - 19 Oct 2019
avatar nikosdion nikosdion - change - 19 Oct 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Oct 2019
Category Administration com_installer Language & Strings SQL Installation Postgresql Libraries Front End Plugins
avatar sanderpotjer sanderpotjer - test_item - 19 Oct 2019 - Tested unsuccessfully
avatar sanderpotjer
sanderpotjer - comment - 19 Oct 2019

I have tested this item ? unsuccessfully on 6a38c97

@nikosdion great PR, thanks for your work on this. Brings some nice improvements for this new functionality.

Note for other testers: discover and enable new "Quick Icon - Missing Download Key Notification" plugin after applying the patch

I've tested this and in general, it works nice, some small bugs/feedback.

Step 2: Would it make sense to link the control panel icon to a filtered list of the "Invalid" entries instead of the supported items? If you have a bunch of extensions it might be easier to see which one needs attention.

Step 3: If no "Invalid entries" are left, filtering for "Download Key invalid" at /administrator/index.php?option=com_installer&view=updatesites results in two notices:
Notice: Undefined index: update_site_id in /libraries/vendor/joomla/database/src/Mysqli/MysqliStatement.php on line 412
Notice: Undefined index: update_site_id in /libraries/vendor/joomla/database/src/Mysqli/MysqliStatement.php on line 413

Step 6: For usability, it might be nice to link from the updates page to an extension edit screen directly to add the missing key, by clicking on the title for example?
Step 6: Can't we force a cache refresh for the extension once a download key is added, to prevent the confusion of the "missing key message"
Step 6: Not sure if the update should work in this test, but it does not ?

Thanks for working on this during PBF19! And greetings to the Greek community ?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26727.

avatar wilsonge
wilsonge - comment - 19 Oct 2019

I know from real world experience that the best way to fix that is to remove all updates and reset the last update check time when the Download Key is saved. If you agree, I can add that behavior to Joomla. I have available time until the end of the month.

Do it please - if this PR hasn't been merged then happy for it to be added in here. If it gets merged before you have time to get it ready then do it separately (depends on if it's going to be a standalone plugin or just a hook in the extension itself partly too).

The notices Sander mentioned need looking at.

His comment about adding a filter in the list view for extensions missing a key also sounds logical - but happy if you'd rather leave that for somewhere else.

Would you like me to do a PR to fix the PHP Codestyle issues here - or are you happy to look at them?

avatar nikosdion
nikosdion - comment - 20 Oct 2019

@sanderpotjer Comments on your findings.

Step 1: Typo in the URL. Thanks! I'll fix it.

Step 3: I cannot reproduce this with Error Reporting set to Development and Debug Site enabled. Can you please turn on logging in your PHP version and check the PHP log for the stack trace of this notice? This is the only item you mentioned I currently can't reproduce or fix.

Step 6: I wanted to link to the Download Key page but the template's CSS was betraying me. If you put a link inside a span.badge-warning you get blue text on dark red background which is illegible. If you only use a link instead of a badge it's not eye-catching enough. Using both a badge and a link it will be, but it looks horrid. I wanted to see if anyone would complain about it before implementing the eyesore solution ?

Regarding cache refresh, that was exactly what I said under "Stretch goal". Based on yours and @wilsonge comments I will implement this ?

As for the update itself, I think I screwed up the URLs in either the manifest or the XML update file. I was developing it with localhost updates, I only created the test domain before sending in the PR. I guess I botched it ? I will look into this and fix it.

avatar nikosdion
nikosdion - comment - 20 Oct 2019

@wilsonge FYI the Joomla code style is buggy. I have to write this n00b-looking code:

		$mapClosure = function (CMSObject $extension) {
			return $extension->get('update_site_id');
		};

		return array_map($mapClosure, $extensions);

instead of

		return array_map(function (CMSObject $extension) {
			return $extension->get('update_site_id');
		}, $extensions);

The problem is that the THIRD line of the best looking code has three code style issues:

  1. It requires putting the closing parenthesis on a new line.
  2. It requires that the closing brace is indented by 8 spaces (2 tabs).
  3. It requires that the closing brace is indented by 12 spaces (3 tabs).
    The first issue makes the resulting code look alien but I can live with that. The second and third issues are mutually exclusive. Therefore, no matter what I do, if I do inline the closure it will always complain about the code style which prevents my PR to be merged.

So I have to write daft code that looks like it was written by a n00b with exactly zero years of actual PHP experience to satisfy the requirement to submit this PR. Therefore, it's a bug.

avatar nikosdion
nikosdion - comment - 20 Oct 2019

Are you guys sure that the Joomla code style makes sense and is a good idea making it a requirement to contribution?

Here is my code that the Joomla code style standard rejects as inscrutable:

		$this->setState('filter.type', $this->getUserStateFromRequest(
			$this->context . '.filter.type', 'filter_type', '', 'string'
		));

The line is too long to write as (phpCS complains!):

		$this->setState('filter.type', $this->getUserStateFromRequest($this->context . '.filter.type', 'filter_type', '', 'string'));

I would also say that single line statements like that are hard to read. The target of the action (filter.type model state) is a mile away from the source (.filter.type context). In the multiline statement the visual proximity enhances comprehension.

Here is the only solution which prevents phpCS from berating me:

		$userState = $this->getUserStateFromRequest($this->context . '.filter.type', 'filter_type', '', 'string');
		$this->setState('filter.type', $userState);

This is even worse than the single line statement. We introduce an intermediate variable which increases the cognitive load of the developer for no reason. Furthermore, the source and target appear in the reverse order of an assignment (the source precedes the target) which further increases cognitive load. Finally, the source and target are also visually separated which adds even more cognitive load. We turned glance-able code to something that requires active mental effort to parse. Why?!

In which possible universe changing mellifluous, coherent and lucid code into a garrulous, obscure and unclear caricature of itself is conducive to better code comprehension by humans? Enforcing Joomla's code style standard is ultimately Kafkaesque, managing to subvert the mission of code style standards into achieving the diametrically opposite result to the intended.

I'll stop rambling about the code style with the note that PHPCBF cannot auto-fix the code, therefore I have to spend a day on doing this manually, delaying actually writing code to fix the issues reported. Unfortunately I do not have the benefit of commit rights which previous contributors abused (existing code violated the code style), so I have to obfuscate my code and the code I found in the files I touched to make tools happy and damn those dirty humans who have the nerve to want to read and understand the source code. This is Joomla and we have to make tools happy.

avatar nikosdion nikosdion - change - 20 Oct 2019
Labels Added: ? ?
avatar HLeithner
HLeithner - comment - 20 Oct 2019

Here is my code that the Joomla code style standard rejects as inscrutable:

		$this->setState('filter.type', $this->getUserStateFromRequest(
			$this->context . '.filter.type', 'filter_type', '', 'string'
		));

The line is too long to write as (phpCS complains!):

		$this->setState('filter.type', $this->getUserStateFromRequest($this->context . '.filter.type', 'filter_type', '', 'string'));

I would also say that single line statements like that are hard to read. The target of the action (filter.type model state) is a mile away from the source (.filter.type context). In the multiline statement the visual proximity enhances comprehension.

Here is the only solution which prevents phpCS from berating me:

		$userState = $this->getUserStateFromRequest($this->context . '.filter.type', 'filter_type', '', 'string');
		$this->setState('filter.type', $userState);

You are right this wouldn't make sense, the code style require to have a multiline
function call starting on it's own line. Your code should look like this:

			$this->setState('filter.type',
				$this->getUserStateFromRequest(
					$this->context . '.filter.type', 'filter_type', '', 'string'
				)
			);
avatar nikosdion
nikosdion - comment - 20 Oct 2019

@HLeithner Please do not ever again rebase my feature branch while I am actively trying to fix reported bugs. When you change the baseline while fixing bugs and see a new issue popping up after fixing a known issue you can not be sure if the new issue is the result of the code you just touched or something else. I shouldn't have to file this comment :/

avatar HLeithner
HLeithner - comment - 20 Oct 2019

@HLeithner Please do not ever again rebase my feature branch while I am actively trying to fix reported bugs. When you change the baseline while fixing bugs and see a new issue popping up after fixing a known issue you can not be sure if the new issue is the result of the code you just touched or something else. I shouldn't have to file this comment :/

I just don't want to bother you updating your branch because it was too many commits behind 4.0-dev to get tested with drone. Sorry for this.

avatar nikosdion
nikosdion - comment - 20 Oct 2019

@HLeithner Next time please let me know in advance. I could keep on working locally without pull / push if I had known. It came as a (rather unpleasant) surprise when my push failed and the pull fetched 60 commits I knew nothing about. Give your devs a heads up, it saves headaches.

Regarding the code style, I agree what my code should look like. Unfortunately the code style standard is asking the penultimate line to have two different indents at the same time ?

Regarding whether I need to touch old code, I agree I shouldn't have to touch old code. However, phpCS not throwing errors is a requirement for the PR being accepted. So I get to fix old code so my new code can be accepted. ?‍♂ Anyway, that's off-topic. I'd be happy to give you and @wilsonge more feedback about it outside this PR if you want. You know my email address.

avatar nikosdion
nikosdion - comment - 20 Oct 2019

@HLeithner I am afraid I am stuck. There are two bugs introduced by the rebase which are unrelated to my code.

Control panel page, Class Joomla\Module\LatestActions\Administrator\Helper\LatestActionsHelper not found in latest actions module.

Going to com_installer directly I get Fatal error: Uncaught TypeError: Argument 1 passed to Symfony\Component\Debug\ExceptionHandler::handle() must be an instance of Exception, instance of Error given in ...../libraries/vendor/symfony/debug/ExceptionHandler.php on line 118

This is exactly why I didn't want to rebase until I'm done bug fixing. All I can do is close this PR, check out the last known good point in my branch and start over :(

avatar HLeithner
HLeithner - comment - 20 Oct 2019

@nikosdion wait a second, the bug is already known. I will try to fix it if no one else did already

avatar nikosdion
nikosdion - comment - 20 Oct 2019

Please do. I have to go cook lunch for the family. I wasted all the time I had this morning on unproductive BS. I don't know when and how much time I will have to actually fix bugs. Hopefully that teaches you a lesson on NOT rebasing other people's branches against their will in the future.

avatar HLeithner
HLeithner - comment - 20 Oct 2019

@nikosdion sorry for the troubles

can you please remove the libraries/autoload_psr4.php and run a composer install

then it seams to work again

avatar brianteeman
brianteeman - comment - 20 Oct 2019

Just removing the libraries/autoload_psr4.php should be enough. It will be regenerated automatically

avatar brianteeman
brianteeman - comment - 20 Oct 2019

PS I share the frustration of other people rebasing while I am working on something

avatar HLeithner
HLeithner - comment - 20 Oct 2019

Filter Options doesn't stay open on reload in updatesites. You have to add 'supported' to the filter_fields list at
https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_installer/Model/UpdatesitesModel.php#L45

avatar nikosdion
nikosdion - comment - 20 Oct 2019

I deleted the autoload_psr4 file BUT the error "Fatal error: Uncaught TypeError: Argument 1 passed to Symfony\Component\Debug\ExceptionHandler::handle() must be an instance of Exception, instance of Error given in ..../libraries/vendor/symfony/debug/ExceptionHandler.php on line 118" remains.

Regarding the Filter Options not staying open: not my monkey, not my problem. I did not touch that code.

Regarding everything else: this was exactly what I was trying to solve when I got stumped by the surprise rebase!

At this point I am too demotivated to continue working on this. I was on a good track and someone else screwed up my branch. Feel free to pick it up. I'm done.

avatar nikosdion nikosdion - change - 20 Oct 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-10-20 10:14:20
Closed_By nikosdion
avatar nikosdion nikosdion - close - 20 Oct 2019
avatar HLeithner
HLeithner - comment - 20 Oct 2019

Just wanted to help, sorry for breaking you dev branch.

avatar nikosdion
nikosdion - comment - 20 Oct 2019

“Sorry” doesn’t change the fact that you killed all the work I had done to the detriment of the time I could have spent with my daughter. That’s the primary reason I am demotivated and fuming. The technical problem is a PITA yet can be addressed. The thing is, the solution is at the detriment of the time I spend with my daughter, again, and it simply takes me back to square one.

If you don’t understand what rebase vs merge is and why rebasing a branch can ne detrimental you shouldn’t have commit rights to begin with. Your position requires you to have an understanding of the tooling and the consequences of “just” pressing a button. You are here to be responsible adults. Start acting like it. It’s not too much to ask.

avatar HLeithner
HLeithner - comment - 20 Oct 2019

You know that I did a merge and not a rebase?

avatar nikosdion
nikosdion - comment - 20 Oct 2019

All I know is my commits are suddenly out of sequence and I see other people's commit between mine. I had to filter the history by author to locate my commits and cherry-pick them on a new branch forked off 4.0-dev to fix my J4 installation. Unfortunately, this means new branch and new PR which is why I closed this one. I COULD try to merge the new branch on top of the old branch and keep the same PR but I can guarantee this will cause merge conflicts and make your life miserable so I am not doing that.

avatar nikosdion nikosdion - change - 20 Oct 2019
The description was changed
avatar nikosdion nikosdion - edited - 20 Oct 2019

Add a Comment

Login with GitHub to post a comment